fix(#29): head-integrity gates — recovery requires exact merged head; reject missing reviewed head (bounce-fix)
Governor review 10539 findings, finding by finding (issue-29 branch, PR #32 retained): 1. Recovery must never attribute a DIFFERENT head's merge to a reviewed-head plan. recover_incomplete() adopted any live record with merged is True for the PR number without comparing heads: if plan A was persisted and then head B was merged externally, the retry row for A would debit/label/close/comment as though reviewed head A had been delivered. Crash-window adoption now requires BOTH positive merged proof AND live head_sha == the persisted planned/reviewed head_sha. A merged DIFFERENT head fails closed with a structured HEAD_MISMATCH result (persisted attempt + result entry), and performs NO debit, NO label removal, NO issue close, NO success comment; the row is left unadopted. The same-head crash-window success case is preserved. 2. A candidate with no reviewed head_sha must never reach the merge client. Passing expected_head_sha=None to merge_pr would arm the production client's backward-compatible live-head fallback and could merge a head the policy never reviewed. _apply_policy() now rejects a missing/empty head_sha BEFORE the plan write (review_state.head_sha is NOT NULL, so the DB would reject it anyway — now with the correct semantic failure) and _safe_merge() independently refuses to call the client without a pinned reviewed head. Zero merge attempts and zero downstream side effects. New regressions (tests/test_review_policy.py): - test_recovery_merged_different_head_fails_closed — plan at sha-A, live merged at sha-B: HEAD_MISMATCH, zero side effects, merged_step unmarked. - test_missing_reviewed_head_never_reaches_merge_client — candidate with no head_sha: structured failure, no merge_pr call, no debit/label/close/comment. Preserved: reviewed-SHA merge pin, production normalized merged/state shape, closed-unmerged negative gate (state==closed never counts), durable recovery independent of the open-PR scan, checkpoint/idempotency coverage, and the same-head crash-window adoption. Evidence: python3 -m pytest -q -> 203 passed, 1 skipped (was 201 passed, 1 skipped; +2 new tests). git diff --check clean. Files: gitea_saas/review_policy.py, tests/test_review_policy.py.
This commit is contained in:
Claude
parent
30e71d44ba
commit
b46c356d64
@ -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)
|
||||
|
||||
@ -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
|
||||
|
||||
Loading…
Reference in New Issue
Block a user