fix(#29): pin merge to reviewed head, positive merged-proof recovery, closed-unmerged guard (bounce-fix)
Governor review #2 findings on PR #32: 1. _safe_merge now passes the REVIEWED head sha to gitea.merge_pr(..., expected_head_sha=...) — the production client (issue #28) fails closed before any POST when the branch moved after review, so an unreviewed SHA can never be merged. Integration regression: SHA A reviewed / live SHA B -> no merge POST, no debit, no close; the moved head is re-reviewed and merged next poll. 2. Production client shape coordinated: GiteaClient._normalize_pr now carries the raw 'merged' boolean and 'state' in every normalized PR (list_open_prs and get_pr), so recover_incomplete can positively identify a merged PR. Tests exercise the production-shaped fake (merged/state fields, open-only list_open_prs, live-record mutation on merge) instead of a richer test-only dict. Crash-window adoption regression: merged_pr checkpoint missed, live record reports merged=True -> recovery finishes debit/label/close/comment without a second merge request. 3. state == 'closed' is no longer treated as merged anywhere: _safe_merge and recover_incomplete adopt a merge only on 'merged is True'. Negative regression: a closed-but-unmerged PR with a pending outcome plan produces no debit, no label removal, no issue close, no success comment. Evidence: 201 passed, 1 skipped (full suite); git diff --check clean.
This commit is contained in:
Claude
parent
d522ea23e4
commit
30e71d44ba
@ -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 []
|
||||
|
||||
@ -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
|
||||
``<repo>#<pr>@<head_sha>``. 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
|
||||
``<repo>#<pr>@<head_sha>``. 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
|
||||
|
||||
@ -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
|
||||
|
||||
Loading…
Reference in New Issue
Block a user