Merge pull request 'gitea-saas-core#30: isolated synthetic-merge review workspace' (#37) from issue-30 into main
This commit is contained in:
commit
71cc2634ce
@ -120,6 +120,8 @@ def load_review_config(env: Optional[Mapping[str, str]] = None) -> dict:
|
||||
| REVIEW_TEST_TIMEOUT_S | test/build timeout seconds | "300" |
|
||||
| REVIEW_SECRET_SCAN | secret scan on/off ("0" disables) | "1" |
|
||||
| REVIEW_WORKTREES_DIR | worktree root where PRs are checked out | (none) |
|
||||
| REVIEW_CLONE_DIR | READ-ONLY repository clone used to build the | (none) |
|
||||
| | isolated synthetic-merge review workspace | |
|
||||
"""
|
||||
source: Mapping[str, str] = os.environ if env is None else env
|
||||
timeout = 300
|
||||
@ -138,4 +140,5 @@ def load_review_config(env: Optional[Mapping[str, str]] = None) -> dict:
|
||||
"test_timeout_s": timeout,
|
||||
"secret_scan": source.get("REVIEW_SECRET_SCAN", "1") != "0",
|
||||
"worktrees_dir": source.get("REVIEW_WORKTREES_DIR") or None,
|
||||
"review_clone_dir": source.get("REVIEW_CLONE_DIR") or None,
|
||||
}
|
||||
|
||||
@ -167,6 +167,7 @@ class GiteaClient:
|
||||
"number": pr.get("number"),
|
||||
"body": pr.get("body") or "",
|
||||
"head_sha": head.get("sha"),
|
||||
"base_sha": base.get("sha"),
|
||||
"base_ref": base.get("ref"),
|
||||
"labels": labels,
|
||||
"repo": repo,
|
||||
@ -262,9 +263,10 @@ class GiteaClient:
|
||||
def get_pr(self, repo: str, number: int) -> dict:
|
||||
"""Fetch a PR in the normalized candidate shape.
|
||||
|
||||
``{number, body, head_sha, base_ref, labels: [names], repo, merged,
|
||||
state, changed_files: [filename, ...]}`` — the review gate consumes
|
||||
``head_sha`` and ``changed_files``; policy consumes the rest
|
||||
``{number, body, head_sha, base_sha, base_ref, labels: [names],
|
||||
repo, merged, state, changed_files: [filename, ...]}`` — the review
|
||||
gate consumes ``head_sha``/``base_sha`` (the exact evaluated pair,
|
||||
issue #30) and ``changed_files``; policy consumes the rest
|
||||
(``merged``/``state`` let recovery positively distinguish a merged
|
||||
PR from a merely closed one).
|
||||
"""
|
||||
|
||||
@ -14,7 +14,8 @@ Seams (all duck-typed, faked in tests):
|
||||
normalized PR shape carries ``merged`` (bool) and ``state`` so the
|
||||
policy can positively distinguish a merged PR from a merely closed one.
|
||||
* ``reviewer`` — the review gate: ``review_pr(repo, pr, linked_issue,
|
||||
repo_dir=...) -> Verdict`` and ``post_verdict(repo, pr, verdict)``.
|
||||
workspace=...) -> Verdict`` (the isolated synthetic-merge workspace,
|
||||
issue #30) and ``post_verdict(repo, pr, verdict)``.
|
||||
|
||||
Policy
|
||||
------
|
||||
@ -33,6 +34,16 @@ head moves. The table is bootstrapped here (``CREATE TABLE IF NOT EXISTS``)
|
||||
because ``schema.sql`` belongs to issue #1's PR and is out of this issue's
|
||||
allowed file set — same schema intent, idempotent.
|
||||
|
||||
Evaluated-pair integrity (issue #30): the reviewer evaluates a synthetic
|
||||
merge of the exact head onto the exact base and the verdict pins both
|
||||
SHAs. Before ANY verdict is applied, the policy re-reads the PR and
|
||||
refuses to apply the verdict unless BOTH the head and the base are still
|
||||
the evaluated pair — a verdict describing a pair that no longer exists is
|
||||
never acted on (no merge, label, comment, or close), and the new pair is
|
||||
simply eligible for a fresh review on the next poll. The merge request
|
||||
itself additionally pins the reviewed head (``expected_head_sha``, issue
|
||||
#28) as a second, tighter guard.
|
||||
|
||||
Transactional outcome application (issue #29): a head is recorded into
|
||||
``reviews`` (i.e. considered COMPLETED) only after its required policy
|
||||
action has fully succeeded. Failure/incomplete attempts are persisted in
|
||||
@ -178,6 +189,14 @@ class ReviewPolicy:
|
||||
self._reviews_conn.execute(_REVIEW_STATE_DDL)
|
||||
self._ensure_review_state_columns()
|
||||
self._reviews_conn.commit()
|
||||
# Isolated review workspace (issue #30): reviews run against a clean
|
||||
# temporary synthetic-merge worktree taken from the configured clone
|
||||
# (REVIEW_CLONE_DIR), never against an arbitrary worktrees root.
|
||||
self._workspace = None
|
||||
clone_dir = self.config.get("review_clone_dir")
|
||||
if clone_dir:
|
||||
from .review_workspace import ReviewWorkspace
|
||||
self._workspace = ReviewWorkspace(clone_dir, self.reviewer.runner)
|
||||
|
||||
def _ensure_review_state_columns(self) -> None:
|
||||
"""Idempotently add columns introduced after the first #29 build."""
|
||||
@ -361,14 +380,48 @@ class ReviewPolicy:
|
||||
pr_body = pr.get("body") or ""
|
||||
issue_body = self._fetch_issue_body(repo, linked_issue_number(pr_body))
|
||||
verdict = self.reviewer.review_pr(
|
||||
repo, pr_number, issue_body,
|
||||
repo_dir=self.config.get("worktrees_dir"))
|
||||
repo, pr_number, issue_body, workspace=self._workspace)
|
||||
except Exception as exc: # noqa: BLE001 - any failure must be retryable
|
||||
self._record_failure(repo_key, pr_number, head_sha, None,
|
||||
self._error_text(exc))
|
||||
results.append(self._failure_result(repo, pr_number, head_sha,
|
||||
None, exc))
|
||||
continue
|
||||
# Evaluated-pair integrity (issue #30): the verdict pins the exact
|
||||
# head/base it evaluated. Re-read the PR BEFORE applying anything
|
||||
# and refuse when either end of the pair moved — a verdict for a
|
||||
# pair that no longer exists must never merge/label/comment/close,
|
||||
# and the new pair stays eligible for a fresh review on the next
|
||||
# poll (nothing is recorded as completed for the stale pair).
|
||||
try:
|
||||
live_pr = self.gitea.get_pr(repo_key, pr_number) or {}
|
||||
pair_moved = (live_pr.get("head_sha") != verdict.head_sha
|
||||
or live_pr.get("base_sha") != verdict.base_sha)
|
||||
except Exception as exc: # noqa: BLE001 - re-read failure is retryable
|
||||
self._record_failure(repo_key, pr_number, head_sha, verdict.verdict,
|
||||
self._error_text(exc))
|
||||
results.append(self._failure_result(repo, pr_number, head_sha,
|
||||
verdict.verdict, exc))
|
||||
continue
|
||||
if pair_moved:
|
||||
results.append({
|
||||
"repo": repo, "pr": pr_number,
|
||||
"head_sha": head_sha, "verdict": verdict.verdict,
|
||||
"outcome": "stale",
|
||||
"error": {
|
||||
"code": "PAIR_MOVED",
|
||||
"message": (
|
||||
f"PR pair moved after evaluation: evaluated head "
|
||||
f"{verdict.head_sha!r} / base {verdict.base_sha!r}, "
|
||||
f"live head {live_pr.get('head_sha')!r} / base "
|
||||
f"{live_pr.get('base_sha')!r} — verdict NOT applied; "
|
||||
"the new pair is eligible for a fresh review")},
|
||||
"evaluated": {"head_sha": verdict.head_sha,
|
||||
"base_sha": verdict.base_sha},
|
||||
"live": {"head_sha": live_pr.get("head_sha"),
|
||||
"base_sha": live_pr.get("base_sha")},
|
||||
})
|
||||
continue
|
||||
try:
|
||||
action = self._apply_policy(repo, pr, pr_body, issue_body, verdict)
|
||||
except Exception as exc: # noqa: BLE001 - any failure must be retryable
|
||||
|
||||
256
gitea_saas/review_workspace.py
Normal file
256
gitea_saas/review_workspace.py
Normal file
@ -0,0 +1,256 @@
|
||||
"""Isolated review workspace (issue #30).
|
||||
|
||||
The review gate must never run tests against an arbitrary ``repo_dir``:
|
||||
a green verdict must describe the exact synthetic merge result it
|
||||
actually evaluated. This module builds that result in a clean,
|
||||
isolated, temporary Git worktree taken from a configured repository
|
||||
clone —
|
||||
|
||||
* the workspace never changes or tests a shared ``main`` checkout or a
|
||||
worker's issue worktree (the configured clone is only ever *read* by
|
||||
``git fetch`` / ``git worktree add``; its checked-out branch and
|
||||
working tree are untouched);
|
||||
* the named base SHA and PR head SHA are fetched into the clone and the
|
||||
exact head is merged onto the exact base (a real merge commit, so
|
||||
tests and ``git diff --check <base> HEAD`` run against a real,
|
||||
inspectable merge result with an explicit base/result range);
|
||||
* a merge conflict produces a deterministic, evidence-carrying result
|
||||
(``merge_ok=False``) and the caller — the review gate — never runs the
|
||||
test suite against some fallback checkout;
|
||||
* the temporary worktree is removed on success, conflict, timeout, and
|
||||
raised exceptions (best-effort ``git worktree remove --force`` plus a
|
||||
belt-and-braces directory removal), and no branch refs are ever
|
||||
created (the worktree is detached), so no temporary refs survive.
|
||||
|
||||
All git commands go through the same ``Runner`` subprocess seam as the
|
||||
rest of the review gate, so hermetic tests can use local-path ``origin``
|
||||
remotes (zero network) and/or a scripted command runner to assert the
|
||||
exact commands, working directories, and ranges.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import re
|
||||
import shlex
|
||||
import shutil
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
from typing import Callable, Optional
|
||||
|
||||
_SHA_RE = re.compile(r"^[0-9a-f]{40}$")
|
||||
|
||||
# Characters kept in a mkdtemp prefix token. ``repo`` names routinely
|
||||
# contain separators (``owner/name``) and punctuation; a prefix is a bare
|
||||
# filename component, so everything else is collapsed to ``-`` (and the
|
||||
# results trimmed) to keep ``tempfile.mkdtemp`` able to create it.
|
||||
_FS_SAFE_RE = re.compile(r"[^A-Za-z0-9_-]+")
|
||||
|
||||
|
||||
def _fs_safe_token(value: str) -> str:
|
||||
"""Collapse ``value`` into a filesystem-safe prefix token."""
|
||||
return _FS_SAFE_RE.sub("-", value).strip("-")
|
||||
|
||||
_MERGE_MESSAGE = "synthetic merge for review"
|
||||
|
||||
|
||||
class ReviewWorkspaceError(Exception):
|
||||
"""The isolated workspace could not be constructed (fetch/resolve
|
||||
failure, missing identity, etc.). Not a merge conflict — the caller
|
||||
treats it as a retryable operational failure."""
|
||||
|
||||
|
||||
class MergeWorkspace:
|
||||
"""A constructed synthetic-merge workspace (context manager).
|
||||
|
||||
Attributes
|
||||
----------
|
||||
dir : Path
|
||||
The temporary worktree (working directory for test/build/hygiene
|
||||
commands). HEAD is the synthetic merge commit of ``head_sha``
|
||||
onto ``base_sha`` when ``merge_ok`` is True.
|
||||
base_sha / head_sha : str
|
||||
The exact SHAs that were merged (what the review evaluates and
|
||||
pins).
|
||||
merge_ok : bool
|
||||
True when the synthetic merge completed; False on conflict.
|
||||
evidence : dict
|
||||
Machine-readable evidence: merge command, exit, output summary,
|
||||
conflicted files (conflict path), result HEAD SHA (clean path).
|
||||
"""
|
||||
|
||||
def __init__(self, dir: Path, base_sha: str, head_sha: str,
|
||||
merge_ok: bool, evidence: dict,
|
||||
cleanup: Optional[Callable[[], None]] = None):
|
||||
self.dir = Path(dir)
|
||||
self.base_sha = base_sha
|
||||
self.head_sha = head_sha
|
||||
self.merge_ok = merge_ok
|
||||
self.evidence = evidence
|
||||
self._cleanup = cleanup
|
||||
|
||||
def __enter__(self) -> "MergeWorkspace":
|
||||
return self
|
||||
|
||||
def __exit__(self, exc_type, exc, tb) -> None:
|
||||
"""Remove the temporary worktree + directory on every path.
|
||||
|
||||
``git worktree remove --force`` cleans the clone's worktree
|
||||
admin entry even when the tree is dirty or the merge left
|
||||
conflict state behind; the directory removal is a belt-and-
|
||||
braces cleanup that never masks the caller's result.
|
||||
"""
|
||||
if self._cleanup is not None:
|
||||
try:
|
||||
self._cleanup()
|
||||
except Exception: # noqa: BLE001 - cleanup must never mask the caller's result
|
||||
pass
|
||||
shutil.rmtree(self.dir, ignore_errors=True)
|
||||
|
||||
def __repr__(self) -> str: # pragma: no cover - debugging aid
|
||||
return (f"<MergeWorkspace {self.dir} merge_ok={self.merge_ok} "
|
||||
f"base={self.base_sha[:12]} head={self.head_sha[:12]}>")
|
||||
|
||||
|
||||
class ReviewWorkspace:
|
||||
"""Builds isolated synthetic-merge workspaces from a configured clone.
|
||||
|
||||
``clone_dir`` is the READ-ONLY configured repository clone (the
|
||||
clone owns the worktree administration; its checked-out branch and
|
||||
working tree are never modified). ``runner`` is the review gate's
|
||||
subprocess seam (``Runner`` or a scripted fake); all git commands
|
||||
run through it so tests stay hermetic.
|
||||
"""
|
||||
|
||||
def __init__(self, clone_dir, runner):
|
||||
self.clone_dir = Path(clone_dir)
|
||||
self.runner = runner
|
||||
|
||||
# -- construction ------------------------------------------------------
|
||||
|
||||
def build(self, repo: str, pr_number: int, head_sha: str,
|
||||
base_sha: str) -> MergeWorkspace:
|
||||
"""Construct the synthetic-merge workspace for one review.
|
||||
|
||||
Raises :class:`ReviewWorkspaceError` on operational failures
|
||||
(unfetchable/unresolvable SHAs, missing git identity, worktree
|
||||
creation failure). Returns a :class:`MergeWorkspace` with
|
||||
``merge_ok=False`` + conflict evidence for a genuine merge
|
||||
conflict, or ``merge_ok=True`` with the merged worktree.
|
||||
"""
|
||||
for label, sha in (("base", base_sha), ("head", head_sha)):
|
||||
if not _SHA_RE.match(sha or ""):
|
||||
raise ReviewWorkspaceError(
|
||||
f"cannot build an isolated review workspace: {label} SHA "
|
||||
f"{sha!r} is not a 40-char hex commit id — refusing to "
|
||||
"review an unchecked-out pair")
|
||||
|
||||
self._fetch_refresh(base_sha, head_sha)
|
||||
|
||||
# ``repo`` is embedded in the temp-dir prefix: sanitize it so a
|
||||
# separator/punctuation-containing name cannot turn the prefix into
|
||||
# a nested path (which would make ``mkdtemp`` fail).
|
||||
tmpdir = Path(tempfile.mkdtemp(
|
||||
prefix=f"review-ws-{_fs_safe_token(repo)}-{pr_number}-"))
|
||||
evidence: dict = {
|
||||
"base_sha": base_sha,
|
||||
"head_sha": head_sha,
|
||||
"worktree_dir": str(tmpdir),
|
||||
}
|
||||
tmpdir_q = shlex.quote(str(tmpdir))
|
||||
try:
|
||||
add = self.runner.run(
|
||||
self.clone_dir,
|
||||
f"git worktree add --detach {tmpdir_q} {base_sha}")
|
||||
if add.get("exit") != 0:
|
||||
raise ReviewWorkspaceError(
|
||||
"cannot create the temporary review worktree: "
|
||||
f"{add.get('summary', '')[:400]}")
|
||||
|
||||
merge = self.runner.run(
|
||||
tmpdir,
|
||||
f"git merge --no-ff -m '{_MERGE_MESSAGE}' {head_sha}")
|
||||
evidence["merge_command"] = (
|
||||
f"git merge --no-ff -m '{_MERGE_MESSAGE}' {head_sha}")
|
||||
evidence["merge_exit"] = merge.get("exit")
|
||||
evidence["merge_summary"] = merge.get("summary", "")[:800]
|
||||
|
||||
if merge.get("exit") != 0:
|
||||
# Distinguish a real merge conflict (MERGE_HEAD exists) from an
|
||||
# environmental merge failure (no conflict state was entered).
|
||||
merget = self.runner.run(
|
||||
tmpdir, "git rev-parse -q --verify MERGE_HEAD")
|
||||
if merget.get("exit") != 0:
|
||||
raise ReviewWorkspaceError(
|
||||
"synthetic merge failed without entering a conflict "
|
||||
f"state (environmental failure): {merge.get('summary', '')[:400]}")
|
||||
conflicted = self.runner.run(
|
||||
tmpdir, "git diff --name-only --diff-filter=U")
|
||||
status = self.runner.run(tmpdir, "git status --porcelain")
|
||||
evidence["conflicted_files"] = [
|
||||
line for line in (conflicted.get("summary") or "").splitlines()
|
||||
if line.strip()]
|
||||
evidence["status_summary"] = (status.get("summary") or "")[:800]
|
||||
return MergeWorkspace(
|
||||
tmpdir, base_sha, head_sha, merge_ok=False,
|
||||
evidence=evidence, cleanup=self._make_cleanup(tmpdir))
|
||||
|
||||
result_head = self.runner.run(tmpdir, "git rev-parse HEAD")
|
||||
evidence["result_head"] = (result_head.get("summary") or "").strip()
|
||||
return MergeWorkspace(
|
||||
tmpdir, base_sha, head_sha, merge_ok=True,
|
||||
evidence=evidence, cleanup=self._make_cleanup(tmpdir))
|
||||
except Exception:
|
||||
# Guarantee cleanup even when construction failed mid-way: a
|
||||
# dangling worktree or temp dir is exactly what must never
|
||||
# survive a review pass.
|
||||
try:
|
||||
self._make_cleanup(tmpdir)()
|
||||
except Exception: # noqa: BLE001 - best-effort cleanup on top of a failure
|
||||
pass
|
||||
shutil.rmtree(tmpdir, ignore_errors=True)
|
||||
raise
|
||||
|
||||
# -- internals ----------------------------------------------------------
|
||||
|
||||
def _make_cleanup(self, tmpdir: Path) -> Callable[[], None]:
|
||||
"""A cleanup closure: ``git worktree remove --force`` + rmtree."""
|
||||
|
||||
# Shell-quoted exactly like construction: an unquoted path with
|
||||
# spaces/shell metacharacters would be split by the shell, git
|
||||
# would fail to remove the worktree's admin entry, and the clone
|
||||
# would keep a dangling worktree record (violating criterion 7).
|
||||
tmpdir_q = shlex.quote(str(tmpdir))
|
||||
|
||||
def cleanup() -> None:
|
||||
try:
|
||||
self.runner.run(
|
||||
self.clone_dir, f"git worktree remove --force {tmpdir_q}")
|
||||
except Exception: # noqa: BLE001 - cleanup is best-effort
|
||||
pass
|
||||
shutil.rmtree(tmpdir, ignore_errors=True)
|
||||
|
||||
return cleanup
|
||||
|
||||
def _fetch_refresh(self, base_sha: str, head_sha: str) -> None:
|
||||
"""Ensure the clone can resolve both SHAs.
|
||||
|
||||
Prefers the precise fetch of exactly the two SHAs; falls back to a
|
||||
full ``git fetch origin`` (which refreshes every remote-tracking
|
||||
ref — the configured clone must be current for reviews anyway) and
|
||||
re-verifies. Hermetic tests use a local-path ``origin``.
|
||||
"""
|
||||
res = self.runner.run(
|
||||
self.clone_dir, f"git fetch origin {base_sha} {head_sha}")
|
||||
if res.get("exit") != 0:
|
||||
res = self.runner.run(self.clone_dir, "git fetch origin")
|
||||
if res.get("exit") != 0:
|
||||
raise ReviewWorkspaceError(
|
||||
"cannot fetch the named base/head SHAs from the configured "
|
||||
f"clone's origin: {res.get('summary', '')[:400]}")
|
||||
for label, sha in (("base", base_sha), ("head", head_sha)):
|
||||
chk = self.runner.run(
|
||||
self.clone_dir, f"git rev-parse --verify {sha}^{{commit}}")
|
||||
if chk.get("exit") != 0:
|
||||
raise ReviewWorkspaceError(
|
||||
f"cannot resolve the {label} SHA {sha} as a commit in the "
|
||||
f"configured clone after fetching: {chk.get('summary', '')[:300]}")
|
||||
@ -9,9 +9,17 @@ Design
|
||||
* ``Reviewer(repo_client, runner)`` — ``repo_client`` is a git/Gitea seam
|
||||
(see ``gitea_client.GiteaClient`` for the real one), ``runner`` is a
|
||||
subprocess seam (``Runner`` here; a scripted fake in tests).
|
||||
* ``review_pr(repo, pr_number, linked_issue, repo_dir)`` returns a
|
||||
* ``review_pr(repo, pr_number, linked_issue, workspace)`` returns a
|
||||
``Verdict``; the caller decides what to do. ``post_verdict`` posts the
|
||||
formatted ``[review-gate]`` comment.
|
||||
formatted ``[review-gate]`` comment. ``workspace`` is the isolated
|
||||
review-workspace seam (``review_workspace.ReviewWorkspace``): the
|
||||
review ALWAYS runs against the synthetic merge result of the exact
|
||||
reviewed head onto the exact reviewed base, constructed in a clean
|
||||
temporary Git worktree — never against an arbitrary ``repo_dir``, a
|
||||
shared ``main`` checkout, or a worker's issue worktree (issue #30).
|
||||
A merge conflict yields a deterministic BOUNCE with evidence and the
|
||||
test suite is NOT run against any fallback checkout; with no
|
||||
workspace configured the gate fails closed to NEEDS_GUIDANCE.
|
||||
* FAIL-CLOSED: the review gate NEVER merges — there is no merge call in
|
||||
this module. Policy and merge are issue #7.
|
||||
* Every finding MUST quote the diff. The default (no LLM configured) pass
|
||||
@ -166,11 +174,20 @@ class Runner:
|
||||
|
||||
@dataclass
|
||||
class Verdict:
|
||||
"""Evidence-based outcome of a review gate run."""
|
||||
"""Evidence-based outcome of a review gate run.
|
||||
|
||||
``base_sha`` pins the base commit the synthetic merge was built on
|
||||
(alongside ``head_sha``) — every review records and pins the
|
||||
repository, PR number, head SHA, and current base SHA it actually
|
||||
evaluated (issue #30). ``workspace`` carries the isolated-workspace
|
||||
evidence (``merge_ok``, ``conflicted_files``, …) when the verdict
|
||||
came from a synthetic-merge review.
|
||||
"""
|
||||
|
||||
repo: str
|
||||
pr: int
|
||||
head_sha: str
|
||||
base_sha: str = ""
|
||||
acceptance: list[str] = field(default_factory=list)
|
||||
tests: dict = field(default_factory=dict)
|
||||
build: Optional[dict] = None
|
||||
@ -178,11 +195,22 @@ class Verdict:
|
||||
findings: list[dict] = field(default_factory=list)
|
||||
verdict: str = NEEDS_GUIDANCE
|
||||
summary: str = ""
|
||||
workspace: dict = field(default_factory=dict)
|
||||
|
||||
def to_comment(self) -> str:
|
||||
"""Format the [review-gate] comment (checklist + findings)."""
|
||||
lines = [f"[review-gate] VERDICT: **{self.verdict}**", "",
|
||||
f"Repo: `{self.repo}` · PR: #{self.pr} · head `{self.head_sha[:12]}`", ""]
|
||||
f"Repo: `{self.repo}` · PR: #{self.pr} · head `{self.head_sha[:12]}`"
|
||||
+ (f" · base `{self.base_sha[:12]}`" if self.base_sha else ""), ""]
|
||||
if self.workspace.get("conflicted_files"):
|
||||
lines += ["Synthetic merge of the reviewed head onto the reviewed base: "
|
||||
"**CONFLICT**",
|
||||
"The test suite was NOT run — no fallback checkout is ever tested.",
|
||||
"Conflicted files:"]
|
||||
lines += [f"- `{f}`" for f in self.workspace["conflicted_files"]]
|
||||
if self.workspace.get("status_summary"):
|
||||
lines += ["```", self.workspace["status_summary"][:400], "```"]
|
||||
lines += [""]
|
||||
lines.append(f"Acceptance parsed ({len(self.acceptance)}):")
|
||||
if self.acceptance:
|
||||
lines += [f"- [ ] {a}" for a in self.acceptance]
|
||||
@ -230,11 +258,24 @@ class Reviewer:
|
||||
|
||||
# -- main entry points -------------------------------------------------
|
||||
|
||||
def review_pr(self, repo: str, pr_number: int, linked_issue, repo_dir) -> Verdict:
|
||||
def review_pr(self, repo: str, pr_number: int, linked_issue,
|
||||
workspace=None) -> Verdict:
|
||||
"""Review one PR against the isolated synthetic-merge workspace.
|
||||
|
||||
``workspace`` is the issue-#30 seam (``ReviewWorkspace``): its
|
||||
``build(repo, pr_number, head_sha, base_sha)`` context manager
|
||||
yields the merged temporary worktree. A green verdict ALWAYS
|
||||
describes that exact merge result — never an arbitrary
|
||||
``repo_dir``. With no workspace (or no resolvable base/head
|
||||
SHAs) the gate fails closed to NEEDS_GUIDANCE; on a merge
|
||||
conflict it returns a deterministic BOUNCE and the test suite is
|
||||
not run.
|
||||
"""
|
||||
issue_body = linked_issue.get("body") if isinstance(linked_issue, dict) else str(linked_issue or "")
|
||||
|
||||
pr = self.repo_client.get_pr(repo, pr_number)
|
||||
head_sha = pr.get("head_sha") or "unknown"
|
||||
base_sha = pr.get("base_sha") or "unknown"
|
||||
changed_files = pr.get("changed_files") or []
|
||||
diff_text = self.repo_client.get_pr_diff(repo, pr_number) or ""
|
||||
|
||||
@ -243,28 +284,58 @@ class Reviewer:
|
||||
scope_violations = [f for f in changed_files
|
||||
if allowed and not _matches_allowed(f, allowed)]
|
||||
|
||||
tests = self.runner.run(repo_dir, "python -m pytest -q",
|
||||
timeout_s=self.config["test_timeout_s"])
|
||||
build = None
|
||||
if self.config.get("build_cmd"):
|
||||
build = self.runner.run(repo_dir, self.config["build_cmd"],
|
||||
if workspace is None:
|
||||
return Verdict(
|
||||
repo=repo, pr=pr_number, head_sha=head_sha, base_sha=base_sha,
|
||||
acceptance=acceptance, tests=dict(exit="not_run"),
|
||||
scope_violations=scope_violations, verdict=NEEDS_GUIDANCE,
|
||||
summary=("no isolated review workspace configured — cannot "
|
||||
"construct and test the synthetic merge; refusing to "
|
||||
"review an arbitrary checkout (never silent-approve)"))
|
||||
|
||||
# The whole review happens inside the workspace: the working tree it
|
||||
# yields IS the synthetic merge of the exact head onto the exact base.
|
||||
with workspace.build(repo, pr_number, head_sha, base_sha) as ws:
|
||||
if not ws.merge_ok:
|
||||
return Verdict(
|
||||
repo=repo, pr=pr_number, head_sha=head_sha, base_sha=base_sha,
|
||||
acceptance=acceptance, tests=dict(
|
||||
exit="not_run",
|
||||
summary="test suite not run: the synthetic merge of the "
|
||||
"reviewed head onto the reviewed base conflicts"),
|
||||
scope_violations=scope_violations, verdict=BOUNCE,
|
||||
summary=("synthetic merge conflict between the reviewed head "
|
||||
"and the current base — see the CONFLICT evidence; "
|
||||
"the test suite was not run against any fallback "
|
||||
"checkout"),
|
||||
workspace={**ws.evidence, "merge_ok": False})
|
||||
|
||||
tests = self.runner.run(ws.dir, "python -m pytest -q",
|
||||
timeout_s=self.config["test_timeout_s"])
|
||||
build["command"] = self.config["build_cmd"]
|
||||
build = None
|
||||
if self.config.get("build_cmd"):
|
||||
build = self.runner.run(ws.dir, self.config["build_cmd"],
|
||||
timeout_s=self.config["test_timeout_s"])
|
||||
build["command"] = self.config["build_cmd"]
|
||||
|
||||
hygiene = self.runner.run(repo_dir, "git diff --check",
|
||||
timeout_s=self.config["test_timeout_s"])
|
||||
# Explicit base/result range: the whitespace gate checks only the
|
||||
# synthetic merge result vs the exact base it was built on.
|
||||
hygiene = self.runner.run(
|
||||
ws.dir, f"git diff --check {ws.base_sha} HEAD",
|
||||
timeout_s=self.config["test_timeout_s"])
|
||||
|
||||
findings = self._collect_findings(diff_text, tests, hygiene)
|
||||
findings = self._collect_findings(diff_text, tests, hygiene)
|
||||
|
||||
verdict, summary = self._decide(
|
||||
issue_body, acceptance, tests, build, scope_violations, findings, hygiene)
|
||||
verdict, summary = self._decide(
|
||||
issue_body, acceptance, tests, build, scope_violations,
|
||||
findings, hygiene)
|
||||
|
||||
return Verdict(
|
||||
repo=repo, pr=pr_number, head_sha=head_sha,
|
||||
acceptance=acceptance, tests=tests, build=build,
|
||||
scope_violations=scope_violations, findings=findings,
|
||||
verdict=verdict, summary=summary,
|
||||
)
|
||||
return Verdict(
|
||||
repo=repo, pr=pr_number, head_sha=ws.head_sha, base_sha=ws.base_sha,
|
||||
acceptance=acceptance, tests=tests, build=build,
|
||||
scope_violations=scope_violations, findings=findings,
|
||||
verdict=verdict, summary=summary,
|
||||
workspace={**ws.evidence, "merge_ok": True})
|
||||
|
||||
def post_verdict(self, repo: str, pr_number: int, verdict: Verdict) -> str:
|
||||
"""Post the formatted [review-gate] comment. Returns the comment."""
|
||||
|
||||
@ -209,6 +209,7 @@ def test_get_pr_normalized_shape_with_changed_files(make_client):
|
||||
assert pr["number"] == 4
|
||||
assert pr["body"] == "Closes #1\nSome PR"
|
||||
assert pr["head_sha"] == "sha-head"
|
||||
assert pr["base_sha"] == "sha-base"
|
||||
assert pr["base_ref"] == "main"
|
||||
assert pr["labels"] == ["in-review", "p1"]
|
||||
assert pr["repo"] == "acme"
|
||||
|
||||
@ -108,9 +108,10 @@ class FakeReviewer:
|
||||
self.verdicts = dict(verdicts)
|
||||
self.calls = []
|
||||
self.posted = []
|
||||
self.runner = None # mirror Reviewer.runner (used to build the workspace)
|
||||
|
||||
def review_pr(self, repo, pr, linked_issue, repo_dir=None):
|
||||
self.calls.append((repo, pr, linked_issue, repo_dir))
|
||||
def review_pr(self, repo, pr, linked_issue, workspace=None):
|
||||
self.calls.append((repo, pr, linked_issue, workspace))
|
||||
verdict = self.verdicts[pr]
|
||||
if callable(verdict):
|
||||
return verdict(repo, pr)
|
||||
@ -121,18 +122,21 @@ class FakeReviewer:
|
||||
return ""
|
||||
|
||||
|
||||
def make_verdict(pr_number, verdict, head_sha="sha-abc"):
|
||||
def make_verdict(pr_number, verdict, head_sha="sha-abc", base_sha="sha-base"):
|
||||
return Verdict(repo="gitea-saas-core", pr=pr_number, head_sha=head_sha,
|
||||
base_sha=base_sha,
|
||||
acceptance=["tests pass"], tests={"exit": 0, "summary": "12 passed"},
|
||||
build=None, scope_violations=[], findings=[],
|
||||
verdict=verdict, summary="ok")
|
||||
|
||||
|
||||
def pr(number, head_sha="sha-abc", labels=("in-review",), body="Closes #1\nSome PR"):
|
||||
def pr(number, head_sha="sha-abc", base_sha="sha-base", labels=("in-review",),
|
||||
body="Closes #1\nSome PR"):
|
||||
# Production-normalized shape (as GiteaClient._normalize_pr returns):
|
||||
# merged bool + state, defaults open / not merged.
|
||||
return {"number": number, "head_sha": head_sha, "labels": list(labels),
|
||||
"body": body, "merged": False, "state": "open"}
|
||||
# merged bool + state + head_sha + base_sha, defaults open / not merged.
|
||||
return {"number": number, "head_sha": head_sha, "base_sha": base_sha,
|
||||
"labels": list(labels), "body": body, "merged": False,
|
||||
"state": "open"}
|
||||
|
||||
|
||||
def tier_issue(tier, number=1):
|
||||
@ -179,7 +183,7 @@ def policy_factory(tmp_path):
|
||||
def test_review_once_per_head_and_skip_same_head(policy_factory):
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(10, head_sha="aaa")], gitea_issues=tier_issue("gold"),
|
||||
verdicts={10: make_verdict(10, APPROVE)})
|
||||
verdicts={10: make_verdict(10, APPROVE, head_sha="aaa")})
|
||||
|
||||
first = policy.poll_and_review("gitea-saas-core")
|
||||
second = policy.poll_and_review("gitea-saas-core")
|
||||
@ -206,6 +210,73 @@ def test_head_move_triggers_rereview(policy_factory):
|
||||
assert [c[1] for c in reviewer.calls] == [10, 10]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Evaluated-pair integrity (issue #30): refuse to apply a verdict whose
|
||||
# head/base pair moved after evaluation
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_evaluated_head_move_refuses_verdict_and_new_pair_gets_fresh_shot(policy_factory):
|
||||
# The reviewer evaluated head "sha-old"; the live PR has already moved
|
||||
# to "sha-new" when the verdict is to be applied. A verdict for a pair
|
||||
# that no longer exists must never merge/label/comment/close — and once
|
||||
# the PR settles on the evaluated head, the pair is fresh-eligible and
|
||||
# the same verdict flow applies normally.
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(50, head_sha="sha-new")], gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={50: make_verdict(50, APPROVE, head_sha="sha-old")})
|
||||
|
||||
results = policy.poll_and_review("gitea-saas-core")
|
||||
r = results[0]
|
||||
assert r["outcome"] == "stale"
|
||||
assert r["error"]["code"] == "PAIR_MOVED"
|
||||
assert r["evaluated"] == {"head_sha": "sha-old", "base_sha": "sha-base"}
|
||||
assert r["live"] == {"head_sha": "sha-new", "base_sha": "sha-base"}
|
||||
assert "not applied" in r["error"]["message"].lower()
|
||||
# zero side effects for the stale pair
|
||||
assert gitea.merged == [] and gitea.comments == []
|
||||
assert gitea.labels_added == [] and gitea.labels_removed == []
|
||||
assert gitea.closed == []
|
||||
assert all(c[3] is None for c in reviewer.calls) # no clone configured -> no workspace
|
||||
|
||||
# The branch settles on the evaluated head: the pair now exists, the
|
||||
# verdict applies (gold tier -> merge), nothing was recorded as complete
|
||||
# for the stale attempt.
|
||||
gitea.prs[0]["head_sha"] = "sha-old"
|
||||
results = policy.poll_and_review("gitea-saas-core")
|
||||
assert results[0]["action"] == "merged"
|
||||
assert gitea.merged == [("gitea-saas-core", 50)]
|
||||
|
||||
|
||||
def test_base_move_after_evaluation_refuses_verdict(policy_factory):
|
||||
# Same head, but the BASE moved after the review evaluated the pair:
|
||||
# the synthetic merge (and therefore the verdict) describes a different
|
||||
# base; refusing is mandatory regardless of head being unchanged.
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(51, head_sha="sha-h", base_sha="sha-base-current")],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={51: make_verdict(51, APPROVE, head_sha="sha-h",
|
||||
base_sha="sha-base-evaluated")})
|
||||
|
||||
results = policy.poll_and_review("gitea-saas-core")
|
||||
r = results[0]
|
||||
assert r["outcome"] == "stale" and r["error"]["code"] == "PAIR_MOVED"
|
||||
assert r["live"] == {"head_sha": "sha-h", "base_sha": "sha-base-current"}
|
||||
assert gitea.merged == [] and gitea.comments == [] and gitea.closed == []
|
||||
|
||||
|
||||
def test_workspace_passed_through_when_clone_configured(policy_factory):
|
||||
# With REVIEW_CLONE_DIR configured, the policy builds the isolated
|
||||
# workspace seam and hands it to the reviewer on every call.
|
||||
ws_sentinel = object()
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(52)], gitea_issues=tier_issue("standard"),
|
||||
verdicts={52: make_verdict(52, APPROVE)},
|
||||
review_clone_dir="/nonexistent/clone")
|
||||
policy._workspace = ws_sentinel # the fixture's clone dir does not exist
|
||||
policy.poll_and_review("gitea-saas-core")
|
||||
assert reviewer.calls[0][3] is ws_sentinel
|
||||
|
||||
|
||||
def test_unlabelled_prs_skipped_unless_review_all(policy_factory):
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(10, labels=("in-review",)), pr(11, labels=())],
|
||||
@ -275,7 +346,7 @@ def test_gold_bounce_adds_label_no_merge_no_debit(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(43, head_sha="sha-b", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={43: make_verdict(43, BOUNCE)})
|
||||
verdicts={43: make_verdict(43, BOUNCE, head_sha="sha-b")})
|
||||
|
||||
results = policy.poll_and_review("gitea-saas-core")
|
||||
|
||||
@ -292,7 +363,7 @@ def test_merge_failure_leaves_pr_open_and_no_close(policy_factory):
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(44, head_sha="sha-c", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={44: make_verdict(44, APPROVE)})
|
||||
verdicts={44: make_verdict(44, APPROVE, head_sha="sha-c")})
|
||||
gitea.merge_results[44] = False
|
||||
|
||||
results = policy.poll_and_review("gitea-saas-core")
|
||||
@ -307,7 +378,7 @@ def test_debit_failure_is_fail_closed(policy_factory):
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(45, head_sha="sha-d", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={45: make_verdict(45, APPROVE)},
|
||||
verdicts={45: make_verdict(45, APPROVE, head_sha="sha-d")},
|
||||
client_ids={})
|
||||
|
||||
results = policy.poll_and_review("gitea-saas-core")
|
||||
@ -325,7 +396,7 @@ def test_standard_approve_comment_only_no_merge(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(50, head_sha="sha-e", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("standard", number=1),
|
||||
verdicts={50: make_verdict(50, APPROVE)})
|
||||
verdicts={50: make_verdict(50, APPROVE, head_sha="sha-e")})
|
||||
|
||||
results = policy.poll_and_review("gitea-saas-core")
|
||||
|
||||
@ -341,7 +412,7 @@ def test_needs_guidance_adds_label_and_comment(policy_factory):
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(51, head_sha="sha-f", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("standard", number=1),
|
||||
verdicts={51: make_verdict(51, NEEDS_GUIDANCE)})
|
||||
verdicts={51: make_verdict(51, NEEDS_GUIDANCE, head_sha="sha-f")})
|
||||
|
||||
results = policy.poll_and_review("gitea-saas-core")
|
||||
|
||||
@ -359,7 +430,7 @@ def test_debit_rate_comes_from_tier_config(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(60, head_sha="sha-g", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={60: make_verdict(60, APPROVE)},
|
||||
verdicts={60: make_verdict(60, APPROVE, head_sha="sha-g")},
|
||||
tiers={**DEFAULT_TIERS, "gold": {"auto_merge": True, "rate_minor": 250}})
|
||||
|
||||
policy.poll_and_review("gitea-saas-core")
|
||||
@ -374,7 +445,7 @@ def test_free_tier_merges_without_debit_when_auto_merge(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(61, head_sha="sha-h", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("free", number=1),
|
||||
verdicts={61: make_verdict(61, APPROVE)},
|
||||
verdicts={61: make_verdict(61, APPROVE, head_sha="sha-h")},
|
||||
tiers={**DEFAULT_TIERS, "free": {"auto_merge": True, "rate_minor": 0}})
|
||||
|
||||
results = policy.poll_and_review("gitea-saas-core")
|
||||
@ -447,7 +518,7 @@ def test_verdict_failure_reported_and_retried(policy_factory):
|
||||
calls["n"] += 1
|
||||
if calls["n"] == 1:
|
||||
raise RuntimeError("verdict generation boom")
|
||||
return make_verdict(pr, APPROVE)
|
||||
return make_verdict(pr, APPROVE, head_sha="sha-v")
|
||||
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(80, head_sha="sha-v", labels=("in-review",))],
|
||||
@ -485,7 +556,7 @@ def test_merge_boundary_failure_retried_merged_once(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(81, head_sha="sha-m", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={81: make_verdict(81, APPROVE)})
|
||||
verdicts={81: make_verdict(81, APPROVE, head_sha="sha-m")})
|
||||
gitea.fail_next["merge_pr"] = RuntimeError("merge boom")
|
||||
|
||||
first = policy.poll_and_review("gitea-saas-core")
|
||||
@ -507,7 +578,7 @@ def test_close_boundary_failure_no_double_merge_or_close(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(82, head_sha="sha-c", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={82: make_verdict(82, APPROVE)})
|
||||
verdicts={82: make_verdict(82, APPROVE, head_sha="sha-c")})
|
||||
gitea.fail_next["close_issue"] = RuntimeError("close boom")
|
||||
|
||||
first = policy.poll_and_review("gitea-saas-core")
|
||||
@ -527,7 +598,7 @@ def test_debit_boundary_failure_never_double_debits(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(83, head_sha="sha-d", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={83: make_verdict(83, APPROVE)})
|
||||
verdicts={83: make_verdict(83, APPROVE, head_sha="sha-d")})
|
||||
real_debit = core.debit
|
||||
calls = {"n": 0}
|
||||
|
||||
@ -555,7 +626,7 @@ def test_label_boundary_failure_bounce_applied_once(policy_factory):
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(84, head_sha="sha-l", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={84: make_verdict(84, BOUNCE)})
|
||||
verdicts={84: make_verdict(84, BOUNCE, head_sha="sha-l")})
|
||||
gitea.fail_next["add_label"] = RuntimeError("label boom")
|
||||
|
||||
first = policy.poll_and_review("gitea-saas-core")
|
||||
@ -574,7 +645,7 @@ def test_comment_boundary_failure_retried(policy_factory):
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(85, head_sha="sha-co", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("standard", number=1),
|
||||
verdicts={85: make_verdict(85, APPROVE)})
|
||||
verdicts={85: make_verdict(85, APPROVE, head_sha="sha-co")})
|
||||
gitea.fail_next["post_comment"] = RuntimeError("comment boom")
|
||||
|
||||
first = policy.poll_and_review("gitea-saas-core")
|
||||
@ -591,7 +662,7 @@ def test_remove_label_boundary_failure_retried(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(86, head_sha="sha-rl", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={86: make_verdict(86, APPROVE)})
|
||||
verdicts={86: make_verdict(86, APPROVE, head_sha="sha-rl")})
|
||||
gitea.fail_next["remove_label"] = RuntimeError("label boom")
|
||||
|
||||
first = policy.poll_and_review("gitea-saas-core")
|
||||
@ -611,7 +682,7 @@ def test_merge_refused_is_retryable_not_completed(policy_factory):
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(87, head_sha="sha-mf", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={87: make_verdict(87, APPROVE)})
|
||||
verdicts={87: make_verdict(87, APPROVE, head_sha="sha-mf")})
|
||||
gitea.merge_results[87] = False
|
||||
|
||||
first = policy.poll_and_review("gitea-saas-core")
|
||||
@ -640,7 +711,7 @@ def test_webhook_after_completed_poll_does_not_double_close(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(88, head_sha="sha-wh", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={88: make_verdict(88, APPROVE)})
|
||||
verdicts={88: make_verdict(88, APPROVE, head_sha="sha-wh")})
|
||||
|
||||
policy.poll_and_review("gitea-saas-core")
|
||||
assert gitea.closed == [("gitea-saas-core", 1)]
|
||||
@ -670,10 +741,17 @@ def test_legacy_review_row_still_skips_head(policy_factory):
|
||||
|
||||
|
||||
def test_failed_attempt_then_head_move_reviews_new_head(policy_factory):
|
||||
# The verdict follows the LIVE head (as the real reviewer's does), so a
|
||||
# failed gold attempt retries at the same head, and after the head moves
|
||||
# the NEW pair is reviewed and merged fresh.
|
||||
def live_head_verdict(repo, pr):
|
||||
return make_verdict(pr, APPROVE,
|
||||
head_sha=gitea.prs[0]["head_sha"])
|
||||
|
||||
policy, gitea, reviewer, _, _ = policy_factory(
|
||||
gitea_prs=[pr(90, head_sha="sha-a", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={90: make_verdict(90, APPROVE)})
|
||||
verdicts={90: live_head_verdict})
|
||||
gitea.fail_next["merge_pr"] = RuntimeError("merge boom")
|
||||
|
||||
first = policy.poll_and_review("gitea-saas-core")
|
||||
@ -703,7 +781,7 @@ def test_merged_pr_gone_from_open_scan_recovered_after_post_merge_failure(policy
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(91, head_sha="sha-91", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={91: make_verdict(91, APPROVE)})
|
||||
verdicts={91: make_verdict(91, APPROVE, head_sha="sha-91")})
|
||||
gitea.fail_next["remove_label"] = RuntimeError("label boom")
|
||||
|
||||
first = policy.poll_and_review("gitea-saas-core")
|
||||
@ -736,7 +814,7 @@ def test_crash_after_label_removed_no_duplicate_visible_actions(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(92, head_sha="sha-92", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={92: make_verdict(92, APPROVE)})
|
||||
verdicts={92: make_verdict(92, APPROVE, head_sha="sha-92")})
|
||||
gitea.fail_next["post_comment"] = RuntimeError("comment boom")
|
||||
|
||||
first = policy.poll_and_review("gitea-saas-core")
|
||||
@ -765,7 +843,7 @@ def test_crash_after_success_comment_before_completion_record(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(93, head_sha="sha-93", labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={93: make_verdict(93, APPROVE)})
|
||||
verdicts={93: make_verdict(93, APPROVE, head_sha="sha-93")})
|
||||
|
||||
real_record = policy._record_completed
|
||||
|
||||
@ -805,14 +883,19 @@ def test_crash_after_success_comment_before_completion_record(policy_factory):
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_reviewed_head_pinned_so_moved_branch_is_never_merged(policy_factory):
|
||||
# Integration regression: review scans SHA A, the branch moves to SHA B
|
||||
# before the merge attempt. The policy must hand the REVIEWED head to
|
||||
# merge_pr, and the production-shaped client seam fails closed: no
|
||||
# merge POST, no debit, no close, no success comment. The moved head is
|
||||
# then a NEW head, re-reviewed and merged normally on the next poll.
|
||||
# Integration regression: review evaluates SHA A, the branch moves to
|
||||
# SHA B before the verdict is applied. The evaluated-pair gate (issue
|
||||
# #30) refuses BEFORE any policy action — no merge POST, no debit, no
|
||||
# close, no success comment. Once the branch sits at the SHAs the
|
||||
# verdict describes, the pair is fresh-eligible and merges normally.
|
||||
state = {"moved": False}
|
||||
|
||||
def review_then_move_head(repo, pr):
|
||||
gitea.prs[0]["head_sha"] = "sha-B" # branch moved after review
|
||||
return make_verdict(pr, APPROVE)
|
||||
evaluated = gitea.prs[0]["head_sha"] # sha-A at review time
|
||||
if not state["moved"]:
|
||||
state["moved"] = True
|
||||
gitea.prs[0]["head_sha"] = "sha-B" # branch moved after review
|
||||
return make_verdict(pr, APPROVE, head_sha=evaluated)
|
||||
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(94, head_sha="sha-A", labels=("in-review",))],
|
||||
@ -820,13 +903,13 @@ def test_reviewed_head_pinned_so_moved_branch_is_never_merged(policy_factory):
|
||||
verdicts={94: review_then_move_head})
|
||||
|
||||
first = policy.poll_and_review("gitea-saas-core")
|
||||
assert first[0]["outcome"] == "failed"
|
||||
assert "refusing to merge" in first[0]["error"]["message"]
|
||||
assert first[0]["outcome"] == "stale"
|
||||
assert first[0]["error"]["code"] == "PAIR_MOVED"
|
||||
assert "sha-A" in first[0]["error"]["message"] and "sha-B" in first[0]["error"]["message"]
|
||||
assert gitea.merged == [] # NO merge POST happened
|
||||
assert gitea.closed == []
|
||||
assert not any("Merged by review-gate" in b for _, _, b in gitea.comments)
|
||||
assert _ledger_debits(core, c["id"]) == [] # no billing for the refused head
|
||||
assert _ledger_debits(core, c["id"]) == [] # no billing for the moved pair
|
||||
|
||||
second = policy.poll_and_review("gitea-saas-core")
|
||||
assert second and second[0]["action"] == "merged" # sha-B reviewed + merged
|
||||
@ -937,7 +1020,7 @@ def test_missing_reviewed_head_never_reaches_merge_client(policy_factory):
|
||||
policy, gitea, reviewer, core, c = policy_factory(
|
||||
gitea_prs=[pr(98, labels=("in-review",))],
|
||||
gitea_issues=tier_issue("gold", number=1),
|
||||
verdicts={98: make_verdict(98, APPROVE)})
|
||||
verdicts={98: make_verdict(98, APPROVE, head_sha=None)})
|
||||
gitea.prs[0]["head_sha"] = None # no reviewed head on the candidate
|
||||
|
||||
results = policy.poll_and_review("gitea-saas-core")
|
||||
|
||||
328
tests/test_review_workspace.py
Normal file
328
tests/test_review_workspace.py
Normal file
@ -0,0 +1,328 @@
|
||||
"""Hermetic tests for the isolated review workspace (issue #30).
|
||||
|
||||
Real ``git`` binaries against LOCAL temporary repositories only — every
|
||||
"remote" is a local-path ``origin`` (a bare repo under ``tmp_path``), so
|
||||
the suite proves the exact production flow (fetch named SHAs, detached
|
||||
worktree, synthetic merge, cleanup) with zero network calls.
|
||||
|
||||
Run with: python -m pytest tests/test_review_workspace.py -q
|
||||
"""
|
||||
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
|
||||
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.review_workspace import ( # noqa: E402
|
||||
ReviewWorkspace,
|
||||
ReviewWorkspaceError,
|
||||
)
|
||||
from gitea_saas.reviewer import Runner # noqa: E402
|
||||
|
||||
GIT = ["git"]
|
||||
|
||||
|
||||
def git(repo, *args):
|
||||
"""Run git in ``repo``; return the CompletedProcess (checked)."""
|
||||
repo = Path(repo)
|
||||
if not repo.exists() and args and args[0] == "init":
|
||||
repo.mkdir(parents=True, exist_ok=True)
|
||||
return subprocess.run(
|
||||
[*GIT, "-C", str(repo), *args], check=True,
|
||||
capture_output=True, text=True)
|
||||
|
||||
|
||||
def git_ok(repo, *args):
|
||||
return git(repo, *args).returncode == 0
|
||||
|
||||
|
||||
def commit_file(repo, filename, content, message):
|
||||
path = repo / filename
|
||||
path.write_text(content)
|
||||
git(repo, "add", filename)
|
||||
git(repo, "commit", "-m", message)
|
||||
return git(repo, "rev-parse", "HEAD").stdout.strip()
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def fixtures(tmp_path):
|
||||
"""Build a local origin (bare), a seeding checkout, and the configured
|
||||
clone. Both the `clean` and the `conflict` head/base pairs come from
|
||||
the same origin (all SHAs are advertised branch tips, so the exact-SHA
|
||||
fetch works over the local-path transport exactly like production)."""
|
||||
origin = tmp_path / "origin.git"
|
||||
git(origin, "init", "--bare", "--initial-branch=main")
|
||||
|
||||
seed = tmp_path / "seed"
|
||||
git(seed, "init", "--initial-branch=main")
|
||||
git(seed, "config", "user.email", "seed@example.com")
|
||||
git(seed, "config", "user.name", "Seed")
|
||||
git(seed, "remote", "add", "origin", str(origin))
|
||||
|
||||
base0 = commit_file(seed, "alpha.txt", "line one\n", "base file")
|
||||
git(seed, "push", "-u", "origin", "main")
|
||||
|
||||
# ---- clean pair: feature adds beta.txt, main adds gamma.txt ----
|
||||
git(seed, "checkout", "-b", "feature", base0)
|
||||
head_clean = commit_file(seed, "beta.txt", "feature add\n", "feature work")
|
||||
git(seed, "push", "-u", "origin", "feature")
|
||||
git(seed, "checkout", "main")
|
||||
base_clean = commit_file(seed, "gamma.txt", "main add\n", "main work")
|
||||
git(seed, "push", "origin", "main")
|
||||
|
||||
# ---- conflict pair: both sides edit alpha.txt after branching ----
|
||||
git(seed, "checkout", "-b", "conflict-head", base0)
|
||||
head_conflict = commit_file(seed, "alpha.txt", "feature change\n", "conflict head")
|
||||
git(seed, "checkout", "main")
|
||||
base_conflict = commit_file(seed, "alpha.txt", "main change\n", "conflict base")
|
||||
git(seed, "push", "origin", "main")
|
||||
git(seed, "push", "-u", "origin", "conflict-head")
|
||||
|
||||
clone = tmp_path / "clone"
|
||||
subprocess.run(["git", "clone", str(origin), str(clone)],
|
||||
check=True, capture_output=True, text=True)
|
||||
git(clone, "config", "user.email", "clone@example.com")
|
||||
git(clone, "config", "user.name", "Clone")
|
||||
clone_head_before = git(clone, "rev-parse", "HEAD").stdout.strip()
|
||||
|
||||
return SimpleNamespace(
|
||||
origin=origin, seed=seed, clone=clone,
|
||||
head_clean=head_clean, base_clean=base_clean,
|
||||
head_conflict=head_conflict, base_conflict=base_conflict,
|
||||
clone_head_before=clone_head_before,
|
||||
)
|
||||
|
||||
|
||||
def review_dirs(pr_number):
|
||||
root = Path(tempfile.gettempdir())
|
||||
return sorted(root.glob(f"review-ws-acme-{pr_number}-*"))
|
||||
|
||||
|
||||
def clone_worktree_lines(clone):
|
||||
return git(clone, "worktree", "list").stdout.strip()
|
||||
|
||||
|
||||
def assert_clone_untouched(fx):
|
||||
assert git(fx.clone, "rev-parse", "HEAD").stdout.strip() == fx.clone_head_before
|
||||
assert git(fx.clone, "status", "--porcelain").stdout.strip() == ""
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Clean merge
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_clean_merge_constructs_synthetic_merge(fixtures):
|
||||
ws = ReviewWorkspace(fixtures.clone, Runner())
|
||||
with ws.build("acme", 1, fixtures.head_clean, fixtures.base_clean) as mws:
|
||||
assert mws.merge_ok is True
|
||||
# The working tree IS the merge result: both sides' files present,
|
||||
# both parent commits in history.
|
||||
assert (mws.dir / "beta.txt").read_text() == "feature add\n"
|
||||
assert (mws.dir / "gamma.txt").read_text() == "main add\n"
|
||||
assert (mws.dir / "alpha.txt").read_text() == "line one\n"
|
||||
parents = git(mws.dir, "rev-list", "--parents", "-n", "1", "HEAD").stdout.split()
|
||||
assert len(parents) == 3 # merge commit + base + head
|
||||
assert mws.evidence["base_sha"] == fixtures.base_clean
|
||||
assert mws.evidence["head_sha"] == fixtures.head_clean
|
||||
assert mws.evidence["result_head"] == parents[0]
|
||||
# Explicit base/result range works from inside the workspace.
|
||||
res = Runner().run(mws.dir, f"git diff --check {fixtures.base_clean} HEAD")
|
||||
assert res["exit"] == 0
|
||||
# Cleaned up after success; the configured clone is untouched.
|
||||
assert review_dirs(1) == []
|
||||
assert "review-ws" not in clone_worktree_lines(fixtures.clone)
|
||||
assert_clone_untouched(fixtures)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Merge conflict
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_merge_conflict_is_deterministic_evidence(fixtures):
|
||||
ws = ReviewWorkspace(fixtures.clone, Runner())
|
||||
with ws.build("acme", 2, fixtures.head_conflict, fixtures.base_conflict) as mws:
|
||||
assert mws.merge_ok is False
|
||||
assert mws.evidence["merge_exit"] != 0
|
||||
assert mws.evidence["conflicted_files"] == ["alpha.txt"]
|
||||
assert "alpha.txt" in mws.evidence["status_summary"]
|
||||
assert review_dirs(2) == []
|
||||
assert "review-ws" not in clone_worktree_lines(fixtures.clone)
|
||||
assert_clone_untouched(fixtures)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# No shared-checkout mutation (source clone AND a worker-style worktree)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_neither_clone_nor_worker_worktree_is_mutated(fixtures):
|
||||
# A worker's issue worktree taken from the same clone must be untouched
|
||||
# by the review pass (its branch, content, and dirty state all survive).
|
||||
worker = fixtures.clone.parent / "worker-issue-42"
|
||||
git(fixtures.clone, "worktree", "add", "--detach", str(worker), fixtures.base_clean)
|
||||
(worker / "alpha.txt").write_text("worker dirty line\n") # simulate WIP
|
||||
worker_head_before = git(worker, "rev-parse", "HEAD").stdout.strip()
|
||||
|
||||
ws = ReviewWorkspace(fixtures.clone, Runner())
|
||||
with ws.build("acme", 3, fixtures.head_clean, fixtures.base_clean) as mws:
|
||||
assert mws.merge_ok is True
|
||||
# the worker checkout's content is untouched by the merge
|
||||
assert (worker / "alpha.txt").read_text() == "worker dirty line\n"
|
||||
assert git(worker, "rev-parse", "HEAD").stdout.strip() == worker_head_before
|
||||
# (strip(): the porcelain status column's leading space is whitespace)
|
||||
assert git(worker, "status", "--porcelain").stdout.strip() == "M alpha.txt"
|
||||
|
||||
assert "review-ws" not in clone_worktree_lines(fixtures.clone)
|
||||
# the worker worktree itself still exists with its dirty state
|
||||
assert worker.exists()
|
||||
assert git(worker, "status", "--porcelain").stdout.strip() == "M alpha.txt"
|
||||
assert_clone_untouched(fixtures)
|
||||
git(fixtures.clone, "worktree", "remove", "--force", str(worker))
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Cleanup on failure paths
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_cleanup_after_raised_exception(fixtures):
|
||||
ws = ReviewWorkspace(fixtures.clone, Runner())
|
||||
before = review_dirs(4)
|
||||
with pytest.raises(RuntimeError, match="boom"):
|
||||
with ws.build("acme", 4, fixtures.head_clean, fixtures.base_clean) as mws:
|
||||
assert mws.merge_ok is True
|
||||
raise RuntimeError("boom")
|
||||
assert review_dirs(4) == before
|
||||
assert "review-ws" not in clone_worktree_lines(fixtures.clone)
|
||||
assert_clone_untouched(fixtures)
|
||||
|
||||
|
||||
def test_cleanup_after_merge_conflict_exit(fixtures):
|
||||
# Simulates the context manager exiting with a conflict result — the
|
||||
# workspace must still be fully removed.
|
||||
ws = ReviewWorkspace(fixtures.clone, Runner())
|
||||
before = review_dirs(5)
|
||||
with ws.build("acme", 5, fixtures.head_conflict, fixtures.base_conflict) as mws:
|
||||
assert mws.merge_ok is False
|
||||
assert review_dirs(5) == before
|
||||
assert "review-ws" not in clone_worktree_lines(fixtures.clone)
|
||||
assert_clone_untouched(fixtures)
|
||||
|
||||
|
||||
def test_unfetchable_sha_raises_and_leaves_nothing(fixtures):
|
||||
ws = ReviewWorkspace(fixtures.clone, Runner())
|
||||
before = review_dirs(6)
|
||||
with pytest.raises(ReviewWorkspaceError):
|
||||
ws.build("acme", 6, "f" * 40, fixtures.base_clean)
|
||||
assert review_dirs(6) == before
|
||||
assert_clone_untouched(fixtures)
|
||||
|
||||
|
||||
def test_non_hex_sha_fails_closed_without_git(fixtures):
|
||||
ws = ReviewWorkspace(fixtures.clone, Runner())
|
||||
with pytest.raises(ReviewWorkspaceError, match="not a 40-char hex"):
|
||||
ws.build("acme", 7, "HEAD", fixtures.base_clean) # a ref, not a SHA
|
||||
with pytest.raises(ReviewWorkspaceError, match="not a 40-char hex"):
|
||||
ws.build("acme", 7, fixtures.head_clean, "main")
|
||||
assert_clone_untouched(fixtures)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Commands run in the right place (cwd + explicit range)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_git_commands_run_in_workspace_cwd(fixtures):
|
||||
calls = []
|
||||
|
||||
def spy(command, **kwargs):
|
||||
calls.append((kwargs.get("cwd"), command))
|
||||
return subprocess.run(command, shell=True, **kwargs)
|
||||
|
||||
runner = Runner(timeout_s=120, command_runner=spy)
|
||||
ws = ReviewWorkspace(fixtures.clone, runner)
|
||||
with ws.build("acme", 8, fixtures.head_clean, fixtures.base_clean) as mws:
|
||||
assert mws.merge_ok is True
|
||||
merges = [c for c in calls if "git merge --no-ff" in c[1]]
|
||||
assert merges, "synthetic merge command must run"
|
||||
assert Path(merges[0][0]) == mws.dir, "merge must run INSIDE the workspace"
|
||||
assert "review-ws" in str(merges[0][0])
|
||||
# fetch + worktree add run against the configured clone (read-only use)
|
||||
adds = [c for c in calls if "worktree add --detach" in c[1]]
|
||||
assert adds and Path(adds[0][0]) == fixtures.clone
|
||||
fetches = [c for c in calls if "git fetch origin" in c[1]]
|
||||
assert fetches and Path(fetches[0][0]) == fixtures.clone
|
||||
# cleanup command targets the clone
|
||||
removes = [c for c in calls if "worktree remove --force" in c[1]]
|
||||
assert removes and Path(removes[0][0]) == fixtures.clone
|
||||
|
||||
|
||||
def test_explicit_range_diff_check_from_workspace(fixtures):
|
||||
calls = []
|
||||
|
||||
def spy(command, **kwargs):
|
||||
calls.append((kwargs.get("cwd"), command))
|
||||
return subprocess.run(command, shell=True, **kwargs)
|
||||
|
||||
runner = Runner(timeout_s=120, command_runner=spy)
|
||||
ws = ReviewWorkspace(fixtures.clone, runner)
|
||||
with ws.build("acme", 9, fixtures.head_clean, fixtures.base_clean) as mws:
|
||||
assert mws.merge_ok is True
|
||||
# The reviewer's hygiene gate: diff --check over the EXPLICIT
|
||||
# base..result range, run from INSIDE the isolated workspace.
|
||||
res = runner.run(mws.dir, f"git diff --check {fixtures.base_clean} HEAD")
|
||||
assert res["exit"] == 0
|
||||
checks = [c for c in calls if "diff --check" in c[1]]
|
||||
assert checks, "explicit-range diff --check must run"
|
||||
assert fixtures.base_clean in checks[0][1] # explicit BASE
|
||||
assert " HEAD" in checks[0][1] # explicit result end
|
||||
assert "review-ws" in str(checks[0][0]) # inside the isolated workspace
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Bounce-fix: shell-safe path quoting + filesystem-safe temp prefix
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_weird_temp_root_with_spaces_and_shell_char_success_and_cleanup(
|
||||
fixtures, tmp_path, monkeypatch):
|
||||
# A valid OS temp root whose path contains a space AND a shell-sensitive
|
||||
# character ($). Unquoted interpolation into the Runner's shell=True
|
||||
# commands would split the path (construction fails) or — worse —
|
||||
# let `git worktree remove` fail while rmtree deletes the directory,
|
||||
# leaving a dangling worktree admin entry in the clone.
|
||||
weird_root = tmp_path / "review root $weird"
|
||||
weird_root.mkdir(parents=True)
|
||||
monkeypatch.setattr(tempfile, "tempdir", str(weird_root))
|
||||
|
||||
ws = ReviewWorkspace(fixtures.clone, Runner())
|
||||
with ws.build("acme", 10, fixtures.head_clean, fixtures.base_clean) as mws:
|
||||
assert mws.merge_ok is True
|
||||
assert mws.dir.exists()
|
||||
assert " " in str(mws.dir) # really lived under the weird root
|
||||
assert "$" in str(mws.dir)
|
||||
assert mws.dir.parent == weird_root
|
||||
# Complete cleanup: directory gone AND no worktree admin record remains.
|
||||
assert not mws.dir.exists()
|
||||
assert "review-ws" not in git(
|
||||
fixtures.clone, "worktree", "list", "--porcelain").stdout
|
||||
assert_clone_untouched(fixtures)
|
||||
|
||||
|
||||
def test_repo_token_with_separators_is_filesystem_safe(fixtures):
|
||||
# ``repo`` is embedded in the mkdtemp prefix; an owner/name-style token
|
||||
# must be collapsed to a single filename component (a NESTED prefix
|
||||
# would make tempfile.mkdtemp raise FileNotFoundError).
|
||||
repo = "pbwraith/gitea-saas-core"
|
||||
ws = ReviewWorkspace(fixtures.clone, Runner())
|
||||
with ws.build(repo, 11, fixtures.head_clean, fixtures.base_clean) as mws:
|
||||
assert mws.merge_ok is True
|
||||
assert mws.dir.name.startswith("review-ws-pbwraith-gitea-saas-core-11-")
|
||||
assert "/" not in mws.dir.name
|
||||
assert not mws.dir.exists()
|
||||
assert "review-ws" not in git(
|
||||
fixtures.clone, "worktree", "list", "--porcelain").stdout
|
||||
assert_clone_untouched(fixtures)
|
||||
@ -11,6 +11,7 @@ import ast
|
||||
import subprocess
|
||||
import sys
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
|
||||
import pytest
|
||||
|
||||
@ -36,13 +37,14 @@ 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.prs = prs or {} # number -> {"head_sha", "base_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"])}
|
||||
return {"head_sha": pr["head_sha"], "base_sha": pr.get("base_sha") or SAMPLE_BASE,
|
||||
"changed_files": list(pr["changed_files"])}
|
||||
|
||||
def get_pr_diff(self, repo, number):
|
||||
return self.prs[number].get("diff", "")
|
||||
@ -54,6 +56,37 @@ class FakeGiteaClient:
|
||||
self.comments.append((repo, number, body))
|
||||
|
||||
|
||||
SAMPLE_BASE = "b" * 40
|
||||
|
||||
|
||||
class FakeWorkspace:
|
||||
"""Duck-typed review-workspace seam: no git, scripted merge result."""
|
||||
|
||||
def __init__(self, merge_ok=True, evidence=None, directory="/fake/ws",
|
||||
base_sha=SAMPLE_BASE):
|
||||
self.merge_ok = merge_ok
|
||||
self.evidence = dict(evidence or {})
|
||||
self.directory = str(directory)
|
||||
self.base_sha = base_sha
|
||||
|
||||
def build(self, repo, pr_number, head_sha, base_sha):
|
||||
return _FakeMergeContext(self)
|
||||
|
||||
|
||||
class _FakeMergeContext:
|
||||
def __init__(self, ws):
|
||||
self._ws = ws
|
||||
|
||||
def __enter__(self):
|
||||
return SimpleNamespace(
|
||||
dir=Path(self._ws.directory), base_sha=self._ws.base_sha,
|
||||
head_sha="a" * 40, merge_ok=self._ws.merge_ok,
|
||||
evidence=self._ws.evidence)
|
||||
|
||||
def __exit__(self, *exc):
|
||||
return False
|
||||
|
||||
|
||||
class FakeRunner:
|
||||
"""Subprocess seam: scripted results, records commands."""
|
||||
|
||||
@ -114,16 +147,19 @@ def test_approve_when_tests_pass_and_no_findings():
|
||||
{"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")
|
||||
verdict = reviewer.review_pr("gitea-saas-core", 1, client.issues[5],
|
||||
workspace=FakeWorkspace())
|
||||
|
||||
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
|
||||
assert verdict.base_sha == "b" * 40
|
||||
|
||||
comment = reviewer.post_verdict("gitea-saas-core", 1, verdict)
|
||||
assert "[review-gate] VERDICT: **APPROVE**" in comment
|
||||
assert "head `aaaaaaaaaaaa`" in comment and "base `bbbbbbbbbbbb`" 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)
|
||||
@ -132,7 +168,8 @@ def test_approve_when_tests_pass_and_no_findings():
|
||||
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")
|
||||
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, client.issues[5],
|
||||
workspace=FakeWorkspace())
|
||||
assert verdict.verdict == BOUNCE
|
||||
assert "tests failed (exit=1)" in verdict.summary
|
||||
# BOUNCE comment carries the test evidence
|
||||
@ -146,7 +183,8 @@ def test_bounce_on_scope_violation_even_with_green_tests():
|
||||
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")
|
||||
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue,
|
||||
workspace=FakeWorkspace())
|
||||
assert verdict.verdict == BOUNCE
|
||||
assert verdict.scope_violations == ["gitea_saas/server.py"]
|
||||
|
||||
@ -155,7 +193,8 @@ 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")
|
||||
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue,
|
||||
workspace=FakeWorkspace())
|
||||
assert verdict.verdict == NEEDS_GUIDANCE
|
||||
assert "never silent-approve" in verdict.summary
|
||||
|
||||
@ -164,7 +203,8 @@ 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")
|
||||
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue,
|
||||
workspace=FakeWorkspace())
|
||||
assert verdict.verdict == APPROVE # tolerated, acceptance present, nothing else blocking
|
||||
|
||||
|
||||
@ -188,7 +228,8 @@ def test_secret_finding_quotes_and_redacts_diff_line():
|
||||
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")
|
||||
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue,
|
||||
workspace=FakeWorkspace())
|
||||
blocking = [f for f in verdict.findings if f["severity"] == "blocking"]
|
||||
assert blocking, "secret scan should produce a blocking finding"
|
||||
f = blocking[0]
|
||||
@ -208,7 +249,7 @@ def test_todo_finding_is_warning_not_blocking():
|
||||
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")
|
||||
"gitea-saas-core", 1, issue, workspace=FakeWorkspace())
|
||||
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"
|
||||
@ -224,7 +265,8 @@ def test_timeout_bounces_with_timed_out_exit():
|
||||
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")
|
||||
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue,
|
||||
workspace=FakeWorkspace())
|
||||
assert verdict.verdict == BOUNCE
|
||||
assert verdict.tests["exit"] == "timed_out"
|
||||
assert "timed out" in verdict.summary
|
||||
@ -270,7 +312,8 @@ def test_build_command_run_when_configured_and_build_failure_bounces():
|
||||
{"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")
|
||||
verdict = reviewer.review_pr("gitea-saas-core", 1, issue,
|
||||
workspace=FakeWorkspace())
|
||||
build = verdict.build
|
||||
assert build is not None
|
||||
assert build["exit"] == 1 and build["command"] == "python -m compileall -q ."
|
||||
@ -283,16 +326,70 @@ def test_diff_hygiene_failure_is_blocking_finding():
|
||||
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")
|
||||
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue,
|
||||
workspace=FakeWorkspace())
|
||||
assert verdict.verdict == BOUNCE
|
||||
assert any("diff --check" in f["note"] for f in verdict.findings)
|
||||
|
||||
|
||||
def test_hygiene_diff_check_uses_explicit_base_head_range():
|
||||
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,
|
||||
workspace=FakeWorkspace())
|
||||
assert verdict.verdict == APPROVE
|
||||
# The whitespace gate runs against the synthetic merge result with an
|
||||
# explicit base..HEAD range (issue #30 / acceptance 5), from the
|
||||
# workspace directory — never a bare `git diff --check`.
|
||||
assert len(runner.commands) == 2 # pytest + explicit-range diff --check
|
||||
ws_dir, hygiene_cmd, _ = runner.commands[1]
|
||||
assert ws_dir == "/fake/ws"
|
||||
assert hygiene_cmd == "git diff --check bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb HEAD"
|
||||
assert "git diff --check" not in hygiene_cmd.replace(
|
||||
"git diff --check bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb HEAD", "")
|
||||
|
||||
|
||||
def test_merge_conflict_bounces_without_running_tests():
|
||||
issue = sample_issue()
|
||||
client = FakeGiteaClient(prs={1: sample_pr()}, issues={5: issue})
|
||||
runner = FakeRunner([{"exit": 0, "summary": "12 passed"}]) # would be tests
|
||||
workspace = FakeWorkspace(merge_ok=False, evidence={
|
||||
"conflicted_files": ["gitea_saas/reviewer.py"],
|
||||
"status_summary": "UU gitea_saas/reviewer.py",
|
||||
"merge_exit": 1})
|
||||
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue,
|
||||
workspace=workspace)
|
||||
assert verdict.verdict == BOUNCE
|
||||
assert "synthetic merge conflict" in verdict.summary
|
||||
assert verdict.tests["exit"] == "not_run"
|
||||
assert runner.commands == [], "the test suite must NOT run on a conflicted merge"
|
||||
assert verdict.workspace["merge_ok"] is False
|
||||
assert verdict.workspace["conflicted_files"] == ["gitea_saas/reviewer.py"]
|
||||
comment = verdict.to_comment()
|
||||
assert "**CONFLICT**" in comment
|
||||
assert "The test suite was NOT run" in comment
|
||||
assert "`gitea_saas/reviewer.py`" in comment
|
||||
|
||||
|
||||
def test_no_workspace_fails_closed_to_needs_guidance():
|
||||
issue = sample_issue()
|
||||
client = FakeGiteaClient(prs={1: sample_pr()}, issues={5: issue})
|
||||
runner = FakeRunner([{"exit": 0, "summary": "12 passed"}])
|
||||
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue)
|
||||
assert verdict.verdict == NEEDS_GUIDANCE
|
||||
assert "no isolated review workspace configured" in verdict.summary
|
||||
assert verdict.tests["exit"] == "not_run"
|
||||
assert runner.commands == [], "no commands may run without a workspace"
|
||||
|
||||
|
||||
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")
|
||||
verdict = Reviewer(client, runner).review_pr("gitea-saas-core", 1, issue,
|
||||
workspace=FakeWorkspace())
|
||||
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
|
||||
|
||||
Loading…
Reference in New Issue
Block a user