diff --git a/gitea_saas/gitea_client.py b/gitea_saas/gitea_client.py index bad6643..65b8b90 100644 --- a/gitea_saas/gitea_client.py +++ b/gitea_saas/gitea_client.py @@ -153,8 +153,11 @@ class GiteaClient: """Map a raw Gitea PR record to the candidate shape. Every candidate supplies at least ``number``, ``body``, ``head_sha``, - ``base_ref``, ``labels`` (label NAMES), and ``repo`` — the shape - ``ReviewPolicy`` and the review gate consume. + ``base_ref``, ``labels`` (label NAMES), ``repo``, ``merged``, and + ``state`` — the shape ``ReviewPolicy`` and the review gate consume. + ``merged``/``state`` are the raw Gitea record's values (``merged`` is + a boolean; only ``merged is True`` proves a merge — ``state == + "closed"`` alone can also mean closed without merging). """ head = pr.get("head") or {} base = pr.get("base") or {} @@ -167,6 +170,8 @@ class GiteaClient: "base_ref": base.get("ref"), "labels": labels, "repo": repo, + "merged": bool(pr.get("merged")), + "state": pr.get("state"), } # -- API surface used by the factory front-end (issue #4) -------------- @@ -257,9 +262,11 @@ 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, - changed_files: [filename, ...]}`` — the review gate consumes - ``head_sha`` and ``changed_files``; policy consumes the rest. + ``{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 + (``merged``/``state`` let recovery positively distinguish a merged + PR from a merely closed one). """ pr = self._request("GET", f"{repo}/pulls/{number}") or {} files = self._request("GET", f"{repo}/pulls/{number}/files") or [] diff --git a/gitea_saas/review_policy.py b/gitea_saas/review_policy.py index 18f56ce..85b8912 100644 --- a/gitea_saas/review_policy.py +++ b/gitea_saas/review_policy.py @@ -9,8 +9,10 @@ Seams (all duck-typed, faked in tests): (see ``billing_core.BillingCore``). * ``gitea`` — Gitea client: ``list_open_prs(repo=None)``, ``get_pr(repo, n)``, ``get_issue(repo, n)``, ``post_comment``, - ``add_label``, ``remove_label``, ``merge_pr(repo, n) -> bool``, - ``close_issue(repo, n)``. + ``add_label``, ``remove_label``, ``merge_pr(repo, n, + expected_head_sha=None) -> bool``, ``close_issue(repo, n)``. The + 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)``. @@ -49,12 +51,19 @@ plan (verdict, tier, linked issue) is persisted into ``review_state`` BEFORE the merge, every post-merge side effect is checkpointed, and ``recover_incomplete()`` resumes those rows directly from the retry table (via ``get_pr`` + checkpoints) — independently of any open-PR listing — -before the regular poll scan runs. Retries re-apply only the steps that -have not already been checkpointed, so a partial side effect can never -double-merge a PR, double-close an issue, double-remove a label, or -double-post the success comment; the delivery debit is additionally -protected by its existing billing-core idempotency key -``#@``. Legacy ``reviews`` rows (pre-#29 schema) are +before the regular poll scan runs. A merge is only ever adopted when the +client reports positive proof (``merged is True``, now part of the +production normalized shape); ``state == "closed"`` without a merge is +never treated as a merge, so a closed-but-unmerged PR cannot trigger a +debit, label removal, issue close, or success comment. Retries re-apply +only the steps that have not already been checkpointed, so a partial side +effect can never double-merge a PR, double-close an issue, double-remove +a label, or double-post the success comment; the delivery debit is +additionally protected by its existing billing-core idempotency key +``#@``. The merge request itself is pinned to the +reviewed head: ``_safe_merge`` passes ``expected_head_sha`` through to the +client so a branch that moved after review is refused before any POST +(issue #28 client contract). Legacy ``reviews`` rows (pre-#29 schema) are preserved untouched and keep their completed meaning; the retry state lives entirely in the new table, so no migration of existing records is needed (new step columns are ALTER-added idempotently for databases @@ -398,8 +407,12 @@ class ReviewPolicy: works for closed/merged PRs) rather than from any open-PR listing. Only rows whose merge is already confirmed (checkpointed, or the - live PR reports merged/closed) are handled here; rows for still- - open PRs are left to the regular poll scan, which can reach them. + live PR positively reports ``merged is True`` — the production + normalized shape includes the raw ``merged`` boolean) are handled + here; rows for still-open PRs are left to the regular poll scan, + which can reach them. ``state == "closed"`` alone never counts as + merged: a PR closed without merging must not trigger a debit, label + removal, issue close, or success comment. Completed heads are recorded into ``reviews`` and their retry row deleted; a step that fails again is persisted with an incremented attempt count for the next recovery pass. @@ -419,8 +432,7 @@ class ReviewPolicy: live = self.gitea.get_pr(row_repo, pr_number) or {} except Exception: # noqa: BLE001 - PR may be gone; checkpoints decide live = None - pr_merged = bool(live and (live.get("merged") is True - or live.get("state") == "closed")) + pr_merged = bool(live and live.get("merged") is True) merged_done = self._step_done(row_repo, pr_number, head_sha, "merged") if pr_merged and not merged_done: @@ -537,19 +549,30 @@ class ReviewPolicy: Never merges twice for the same head: the ``merged_step`` checkpoint makes a retry skip an already-applied merge, and when the fetched PR - shape already reports ``merged``/``state=closed`` (crash window + shape already positively reports ``merged is True`` (crash window between the API side effect and the checkpoint write) the merge is - skipped as well. + skipped as well. ``state == "closed"`` alone is NOT adopted — a PR + can be closed without merging, and adopting it would debit the + client, strip labels, and close the linked issue for a merge that + never happened. + + The merge request is pinned to the REVIEWED head: the exact + ``head_sha`` this policy reviewed is passed through to + ``gitea.merge_pr(..., expected_head_sha=...)``, whose production + implementation (issue #28) fails closed before any POST when the + live branch moved after review — an unreviewed SHA can never be + merged. """ repo_key = repo or "" pr_number = pr["number"] head_sha = pr.get("head_sha") or "unknown" if self._step_done(repo_key, pr_number, head_sha, "merged"): return True - if pr.get("merged") is True or pr.get("state") == "closed": + if pr.get("merged") is True: self._mark_step(repo_key, pr_number, head_sha, "merged") return True - merged = self.gitea.merge_pr(repo, pr_number) + merged = self.gitea.merge_pr( + repo, pr_number, expected_head_sha=pr.get("head_sha")) if merged: self._mark_step(repo_key, pr_number, head_sha, "merged") return merged diff --git a/tests/test_review_policy.py b/tests/test_review_policy.py index 08aa5df..8bf0f7d 100644 --- a/tests/test_review_policy.py +++ b/tests/test_review_policy.py @@ -27,10 +27,19 @@ from gitea_saas.reviewer import Verdict # noqa: E402 class FakeGitea: - """In-memory Gitea seam recording every mutation.""" + """In-memory Gitea seam recording every mutation. + + Mirrors the production client's NORMALIZED shape (issue #28/#31): + PRs carry ``merged`` (bool) and ``state`` exactly like + ``GiteaClient._normalize_pr`` output, ``list_open_prs`` returns only + open-unmerged PRs, ``get_pr`` returns the live record, and a + successful ``merge_pr`` flips the live record to ``merged=True`` / + ``state="closed"`` — so tests exercise the production-shaped dict, not + a richer test-only one. + """ def __init__(self, prs=None, issues=None): - self.prs = list(prs or []) # [{number, head_sha, labels, body}] + self.prs = list(prs or []) # [{number, head_sha, labels, body, merged, state}] self.issues = dict(issues or {}) # number -> {"body": ...} self.comments = [] self.labels_added = [] @@ -50,7 +59,9 @@ class FakeGitea: return next(p for p in self.prs if p["number"] == number) def list_open_prs(self, repo=None): - return [dict(p) for p in self.prs] + # Production behavior: ?state=open returns open, unmerged PRs only. + return [dict(p) for p in self.prs + if p.get("state") == "open" and not p.get("merged")] def get_pr(self, repo, number): return dict(self._pr(number)) @@ -70,10 +81,20 @@ class FakeGitea: self._maybe_fail("remove_label") self.labels_removed.append((repo, number, label)) - def merge_pr(self, repo, number): + def merge_pr(self, repo, number, expected_head_sha=None): self._maybe_fail("merge_pr") + live = self._pr(number) + if expected_head_sha is not None and live.get("head_sha") != expected_head_sha: + raise RuntimeError( + f"merge_pr: PR {number} head is {live.get('head_sha')!r} but the " + f"reviewed head is {expected_head_sha!r}; refusing to merge " + "a branch that moved after review") self.merged.append((repo, number)) - return self.merge_results.get(number, True) + merged = self.merge_results.get(number, True) + if merged: + live["merged"] = True + live["state"] = "closed" + return merged def close_issue(self, repo, number): self._maybe_fail("close_issue") @@ -108,7 +129,10 @@ def make_verdict(pr_number, verdict, head_sha="sha-abc"): def pr(number, head_sha="sha-abc", labels=("in-review",), body="Closes #1\nSome PR"): - return {"number": number, "head_sha": head_sha, "labels": list(labels), "body": body} + # 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"} def tier_issue(tier, number=1): @@ -166,8 +190,12 @@ def test_review_once_per_head_and_skip_same_head(policy_factory): def test_head_move_triggers_rereview(policy_factory): + # Standard tier keeps the PR OPEN after the first poll (comment only), + # so a head move is observable by the next scan and must re-review. + # (A gold-tier poll merges, and a merged PR leaves the open scan — the + # production-shaped fake reflects that.) policy, gitea, reviewer, _, _ = policy_factory( - gitea_prs=[pr(10, head_sha="aaa")], gitea_issues=tier_issue("gold"), + gitea_prs=[pr(10, head_sha="aaa")], gitea_issues=tier_issue("standard"), verdicts={10: make_verdict(10, APPROVE)}) policy.poll_and_review("gitea-saas-core") @@ -764,3 +792,98 @@ def test_crash_after_success_comment_before_completion_record(policy_factory): assert gitea.closed == [("gitea-saas-core", 1)] assert len(_ledger_debits(core, c["id"])) == 1 assert policy.poll_and_review("gitea-saas-core") == [] + + +# --------------------------------------------------------------------------- +# Governor review #2 regressions: +# 1. the merge is pinned to the REVIEWED head (expected_head_sha) — a branch +# that moved after review is refused before any merge POST; +# 2. recovery distinguishes a merged PR from a merely closed one using the +# PRODUCTION normalized shape (positive merged is True only); +# 3. a closed-but-unmerged PR is never adopted as merged (no debit, no label +# removal, no issue close, no success comment). +# --------------------------------------------------------------------------- + +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. + def review_then_move_head(repo, pr): + gitea.prs[0]["head_sha"] = "sha-B" # branch moved after review + return make_verdict(pr, APPROVE) + + policy, gitea, reviewer, core, c = policy_factory( + gitea_prs=[pr(94, head_sha="sha-A", labels=("in-review",))], + gitea_issues=tier_issue("gold", number=1), + 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 "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 + + second = policy.poll_and_review("gitea-saas-core") + assert second and second[0]["action"] == "merged" # sha-B reviewed + merged + assert gitea.merged == [("gitea-saas-core", 94)] # exactly one merge, ever + assert gitea.closed == [("gitea-saas-core", 1)] + assert len(_ledger_debits(core, c["id"])) == 1 + assert policy._already_reviewed("gitea-saas-core", 94, "sha-A") is False + assert policy._already_reviewed("gitea-saas-core", 94, "sha-B") is True + + +def test_crash_window_adopts_merged_pr_from_production_shape(policy_factory): + # The merge applied server-side but the process died before the + # merged_step checkpoint write: with the production normalized shape + # (merged/state now included), recovery positively adopts the merged PR + # and finishes debit/label/close/comment — without ever calling + # merge_pr again. + policy, gitea, reviewer, core, c = policy_factory( + gitea_prs=[pr(96, head_sha="sha-96", labels=("in-review",))], + gitea_issues=tier_issue("gold", number=1), + verdicts={}) + policy._ensure_plan("gitea-saas-core", 96, "sha-96", APPROVE, "gold", 1) + # The live PR record already reports the server-side merge (raw Gitea + # record normalized by the production client); the checkpoint is the + # only thing missing. + gitea.prs[0]["merged"] = True + gitea.prs[0]["state"] = "closed" + + results = policy.poll_and_review("gitea-saas-core") + assert results and results[0]["action"] == "merged" + assert results[0].get("recovered") is True + assert gitea.merged == [] # crash window: no merge request needed + assert gitea.labels_removed.count(("gitea-saas-core", 96, "in-review")) == 1 + assert gitea.closed == [("gitea-saas-core", 1)] + assert any("Merged by review-gate" in b for _, _, b in gitea.comments) + assert len(_ledger_debits(core, c["id"])) == 1 + assert policy._already_reviewed("gitea-saas-core", 96, "sha-96") is True + assert policy.poll_and_review("gitea-saas-core") == [] # completed: skipped + + +def test_closed_unmerged_pr_recovery_never_debits_or_closes(policy_factory): + # Negative regression: state == "closed" is NOT proof of a merge. The + # outcome plan exists for the head, the merge never happened, and the + # PR is closed without merging — recovery must not adopt it as merged: + # no debit, no label removal, no issue close, no success comment. + policy, gitea, reviewer, core, c = policy_factory( + gitea_prs=[pr(95, head_sha="sha-95", labels=("in-review",))], + gitea_issues=tier_issue("gold", number=1), + verdicts={}) + policy._ensure_plan("gitea-saas-core", 95, "sha-95", APPROVE, "gold", 1) + gitea.prs[0]["state"] = "closed" # closed WITHOUT merging + gitea.prs[0]["merged"] = False + + results = policy.poll_and_review("gitea-saas-core") + assert results == [] # nothing adopted, nothing applied + assert gitea.merged == [] and gitea.closed == [] + assert gitea.labels_removed == [] + assert gitea.comments == [] + assert _ledger_debits(core, c["id"]) == [] + assert policy._step_done("gitea-saas-core", 95, "sha-95", "merged") is False + assert policy._already_reviewed("gitea-saas-core", 95, "sha-95") is False