diff --git a/gitea_saas/review_policy.py b/gitea_saas/review_policy.py index 85b8912..73656ac 100644 --- a/gitea_saas/review_policy.py +++ b/gitea_saas/review_policy.py @@ -407,12 +407,17 @@ 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 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. + live PR positively reports ``merged is True`` at the EXACT + planned/reviewed head — the production normalized shape includes + the raw ``merged`` boolean and ``head_sha``) 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. A PR merged at a DIFFERENT head + than the plan reviewed is never adopted — it fails closed with a + structured ``HEAD_MISMATCH`` and no side effects (the debit, + label, close and comment steps all belong to the reviewed head, + not to whatever head actually landed). 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. @@ -437,7 +442,32 @@ class ReviewPolicy: "merged") if pr_merged and not merged_done: # Crash window: the merge applied server-side but the - # checkpoint write never happened — adopt it. + # checkpoint write never happened — adopt it ONLY when + # the live merged head is the EXACT planned/reviewed + # head (head-integrity gate, governor 10539). A merge + # of a DIFFERENT head must never be attributed to this + # plan's debit/label/close/comment steps. + live_head = (live or {}).get("head_sha") + if live_head != head_sha: + message = ( + f"merged head mismatch: live head {live_head!r} " + f"!= planned/reviewed head {head_sha!r} — the " + f"outcome application for the reviewed head is " + f"refused (no debit, no label removal, no issue " + f"close, no success comment)") + self._record_failure(row_repo, pr_number, head_sha, + verdict, message) + results.append({ + "repo": row_repo, "pr": pr_number, + "head_sha": head_sha, "verdict": verdict, + "outcome": "failed", + "error": {"code": "HEAD_MISMATCH", + "message": message}, + "attempts": self._attempts( + row_repo, pr_number, head_sha), + }) + continue + # Same-head crash window: adopt and finish the plan. self._mark_step(row_repo, pr_number, head_sha, "merged") merged_done = True if not merged_done: @@ -561,18 +591,30 @@ class ReviewPolicy: ``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. + merged. A candidate that carries NO reviewed head_sha is rejected + BEFORE the client is called (head-integrity gate, governor 10539): + ``expected_head_sha=None`` would arm the client's backward- + compatible live-head fallback and could merge a head this policy + never reviewed, so the merge is refused up front and the failure + is surfaced as a structured retryable result with zero side + effects. """ repo_key = repo or "" pr_number = pr["number"] - head_sha = pr.get("head_sha") or "unknown" + reviewed_head = pr.get("head_sha") or "" + if not reviewed_head: + raise ValueError( + "merge refused: candidate PR carries no reviewed head_sha — " + "expected_head_sha must be pinned; refusing to fall back to " + "the live head") + head_sha = reviewed_head if self._step_done(repo_key, pr_number, head_sha, "merged"): return True 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, expected_head_sha=pr.get("head_sha")) + repo, pr_number, expected_head_sha=reviewed_head) if merged: self._mark_step(repo_key, pr_number, head_sha, "merged") return merged @@ -621,7 +663,16 @@ class ReviewPolicy: # truly merged it leaves the open-PR scan, so every following # step must be recoverable from the row alone (issue #29 / # review bounce: merged PRs vanish from list_open_prs()). - head_sha = pr.get("head_sha") + head_sha = pr.get("head_sha") or "" + if not head_sha: + # Head-integrity gate (governor 10539): a candidate without + # a reviewed head_sha is rejected BEFORE any plan write or + # client call — expected_head_sha must be pinned, never the + # client's live-head fallback. + raise ValueError( + "merge refused: candidate PR carries no reviewed head_sha — " + "expected_head_sha must be pinned; refusing to fall back to " + "the live head") issue_num = linked_issue_number(pr_body) self._ensure_plan(repo, pr_number, head_sha, verdict.verdict, tier, issue_num) diff --git a/tests/test_review_policy.py b/tests/test_review_policy.py index 8bf0f7d..8714b30 100644 --- a/tests/test_review_policy.py +++ b/tests/test_review_policy.py @@ -887,3 +887,65 @@ def test_closed_unmerged_pr_recovery_never_debits_or_closes(policy_factory): 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 + + +# --------------------------------------------------------------------------- +# Governor review #3 (10539) regressions — head-integrity gates: +# 1. recovery adopts a merged PR ONLY when the live merged head equals the +# persisted planned/reviewed head; a DIFFERENT merged head fails closed +# with a structured HEAD_MISMATCH and no debit/label/close/comment; +# 2. a candidate with NO reviewed head_sha is rejected BEFORE the merge +# client is called (expected_head_sha=None would arm the client's +# live-head fallback and could merge an unreviewed head). +# --------------------------------------------------------------------------- + +def test_recovery_merged_different_head_fails_closed(policy_factory): + # Negative regression: the plan was persisted for reviewed head sha-A, + # but the branch moved and head sha-B was merged externally before the + # merged_step checkpoint write. Recovery must NOT attribute the sha-B + # merge to the sha-A plan: zero side effects (no debit, no label + # removal, no issue close, no success comment), a structured + # HEAD_MISMATCH result, and the row left unadopted for the operator. + policy, gitea, reviewer, core, c = policy_factory( + gitea_prs=[pr(97, head_sha="sha-A", labels=("in-review",))], + gitea_issues=tier_issue("gold", number=1), + verdicts={}) + policy._ensure_plan("gitea-saas-core", 97, "sha-A", APPROVE, "gold", 1) + gitea.prs[0]["head_sha"] = "sha-B" # branch moved AND merged externally + gitea.prs[0]["merged"] = True + gitea.prs[0]["state"] = "closed" + + results = policy.poll_and_review("gitea-saas-core") + assert results and results[0]["outcome"] == "failed" + assert results[0]["error"]["code"] == "HEAD_MISMATCH" + assert "sha-A" in results[0]["error"]["message"] + assert "sha-B" in results[0]["error"]["message"] + 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", 97, "sha-A", "merged") is False + assert policy._already_reviewed("gitea-saas-core", 97, "sha-A") is False + + +def test_missing_reviewed_head_never_reaches_merge_client(policy_factory): + # Negative regression: a candidate PR with no head_sha must be refused + # BEFORE the merge client is called — passing expected_head_sha=None + # would arm the production client's live-head fallback and could merge + # a head the policy never reviewed. Assert zero merge attempts and zero + # billing/label/close/comment side effects, with a structured failure. + 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)}) + gitea.prs[0]["head_sha"] = None # no reviewed head on the candidate + + results = policy.poll_and_review("gitea-saas-core") + assert results and results[0]["outcome"] == "failed" + assert "no reviewed head_sha" in results[0]["error"]["message"] + assert "fall back to the live head" in results[0]["error"]["message"] + assert gitea.merged == [] and gitea.closed == [] + assert gitea.labels_removed == [] + assert gitea.comments == [] + assert _ledger_debits(core, c["id"]) == [] + assert policy._already_reviewed("gitea-saas-core", 98, "unknown") is False