diff --git a/.gitea/scripts/gitea-merge-queue.py b/.gitea/scripts/gitea-merge-queue.py index 17c3d318e..3aa0edf64 100644 --- a/.gitea/scripts/gitea-merge-queue.py +++ b/.gitea/scripts/gitea-merge-queue.py @@ -1,16 +1,40 @@ #!/usr/bin/env python3 """gitea-merge-queue — conservative serialized merge bot for Gitea. -Gitea 1.22.6 has auto-merge (`pull_auto_merge`) but no GitHub-style merge +Gitea 1.22.6+ has auto-merge (`pull_auto_merge`) but no GitHub-style merge queue. This script provides the missing serialized policy in user space: -1. Pick the oldest open PR carrying QUEUE_LABEL. -2. Refuse to act unless main is green. +1. Pick the oldest open PR carrying QUEUE_LABEL (skipping HOLD_LABEL). +2. Refuse to act unless main's BP-required contexts are green. 3. Refuse fork PRs; the queue may only mutate same-repo branches. 4. If the PR branch does not contain current main, call Gitea's /pulls/{n}/update endpoint and stop. CI must rerun on the updated head. -5. If the updated PR head has all required contexts green, merge with the - non-bypass merge actor token. +5. Merge ONLY when, on the PR's CURRENT head sha: + - >= REQUIRED_APPROVALS distinct GENUINE official APPROVED reviews from + the recognised reviewer set (not stale, not dismissed, commit_id == + current head), AND + - no open official REQUEST_CHANGES on the current head, AND + - every BP-required status context is green, AND + - the PR is mergeable. + +Authoritative gates (fail-closed): + - The REQUIRED status contexts come from BRANCH PROTECTION + (`status_check_contexts`), not a hand-maintained env list. If branch + protection cannot be enumerated, the queue HOLDS (does not merge blindly). + - NON-required reds (qa-review, security-review, sop-tier, sop-checklist + when not branch-required, E2E Chat, Staging SaaS, ci-arm64-advisory, any + continue-on-error job) MUST NOT block. They are reported, never gating. + - `force_merge=true` is used ONLY when the merge is blocked *solely* by + missing-but-non-required governance contexts (required are green + genuine + approvals present). It is NEVER used to bypass a failing REQUIRED context + or missing approvals. + +Head-of-line (HOL) safety: a permanent permission/4xx merge error +(403/404/405) HOLDS the PR (applies HOLD_LABEL) so the queue advances to the +next PR instead of re-selecting the same wedged PR every tick. + +Status-fetch is fail-closed: if the combined status for a sha cannot be +fetched, the PR is skipped this tick (never treated as green). The script is intentionally one-PR-per-run. Workflow/cron concurrency should serialize invocations so two green PRs cannot merge against the same main. @@ -57,6 +81,24 @@ PUSH_REQUIRED_CONTEXTS_RAW = _env( default="CI / all-required (push)", ) +# Recognised official-reviewer set. A merge requires this many DISTINCT genuine +# approvals (not stale/dismissed, on the current head sha) from accounts in +# this set. The set is the real agents-team reviewer roster; founder/CTO-agent +# accounts are intentionally excluded so the queue cannot be satisfied by a +# human/owner approval alone — it must be a genuine peer review. +REVIEWER_SET = { + name.strip() + for name in _env( + "REVIEWER_SET", + default="agent-reviewer,agent-researcher,agent-reviewer-cr2", + ).split(",") + if name.strip() +} +# Default mirrors molecule-core branch protection (required_approvals: 2). The +# authoritative value is read from branch protection at runtime; this is only +# the fallback when BP does not specify one. +REQUIRED_APPROVALS_DEFAULT = int(_env("REQUIRED_APPROVALS", default="2") or "2") + OWNER, NAME = (REPO.split("/", 1) + [""])[:2] if REPO else ("", "") API = f"https://{GITEA_HOST}/api/v1" if GITEA_HOST else "" @@ -67,7 +109,13 @@ class ApiError(RuntimeError): class MergePermissionError(ApiError): """Merge failed with a permanent permission error (403/404/405). - The queue should skip this PR and move to the next one.""" + The queue should HOLD this PR and move to the next one.""" + + +class BranchProtectionUnavailable(ApiError): + """Branch protection (the authoritative required-context source) could not + be enumerated. The queue must HOLD rather than merge with an unverified + required-context set (fail-closed, no fail-open).""" @dataclasses.dataclass(frozen=True) @@ -75,6 +123,20 @@ class MergeDecision: ready: bool action: str reason: str + # When ready is True, force indicates the merge is blocked SOLELY by + # missing-but-non-required governance contexts (required are green + + # genuine approvals present), so force_merge=true is justified to bypass + # ONLY those non-required contexts. Defaults False. + force: bool = False + + +@dataclasses.dataclass(frozen=True) +class BranchProtection: + """The subset of branch protection the queue depends on.""" + + required_contexts: list[str] + required_approvals: int + block_on_rejected_reviews: bool def _require_runtime_env() -> None: @@ -191,6 +253,117 @@ def required_contexts_green( return not missing_or_bad, missing_or_bad +def parse_branch_protection(body: Any) -> BranchProtection: + """Extract the queue-relevant fields from a branch_protections payload. + + Fail-closed: raises BranchProtectionUnavailable when status checks are + expected but the required-context list cannot be enumerated. We never fall + back to a hand-maintained env list as the authoritative required set — + doing so risks merging when a real required context is red/missing. + """ + if not isinstance(body, dict): + raise BranchProtectionUnavailable("branch protection response not an object") + enable = bool(body.get("enable_status_check")) + contexts_raw = body.get("status_check_contexts") + if not enable: + # Status checks not enforced by BP at all. With no required contexts + # the queue would gate on approvals only — acceptable, but make it + # explicit and let the caller decide. + contexts: list[str] = [] + else: + if not isinstance(contexts_raw, list): + raise BranchProtectionUnavailable( + "enable_status_check is true but status_check_contexts is not a list" + ) + contexts = [c for c in contexts_raw if isinstance(c, str) and c.strip()] + if not contexts: + raise BranchProtectionUnavailable( + "enable_status_check is true but status_check_contexts is empty" + ) + approvals = body.get("required_approvals") + required_approvals = ( + int(approvals) if isinstance(approvals, int) else REQUIRED_APPROVALS_DEFAULT + ) + return BranchProtection( + required_contexts=contexts, + required_approvals=required_approvals, + block_on_rejected_reviews=bool(body.get("block_on_rejected_reviews")), + ) + + +def get_branch_protection(branch: str) -> BranchProtection: + """Fetch branch protection for `branch`; fail-closed if unavailable.""" + try: + _, body = api("GET", f"/repos/{OWNER}/{NAME}/branch_protections/{branch}") + except ApiError as exc: + raise BranchProtectionUnavailable( + f"could not fetch branch protection for {branch}: {exc}" + ) from exc + return parse_branch_protection(body) + + +def genuine_approvals( + reviews: list[dict], + *, + head_sha: str, + reviewer_set: set[str], +) -> tuple[set[str], list[str]]: + """Reduce a PR's reviews to genuine official approvals on the CURRENT head. + + Returns (approvers, request_changes) where: + - approvers is the set of distinct logins (in reviewer_set) whose LATEST + review on the current head is an official, non-stale, non-dismissed + APPROVED, and + - request_changes is the list of logins (in reviewer_set) whose latest + official review on the current head is REQUEST_CHANGES. + + "Current head" is enforced two ways, because Gitea exposes both signals: + a review must be `official` and NOT `stale`/`dismissed`, AND when the + review carries a commit_id it must equal head_sha. A review with no + commit_id but stale=False/dismissed=False is accepted (older Gitea rows). + We take each reviewer's LATEST submission (reviews arrive oldest-first), so + a later REQUEST_CHANGES correctly supersedes an earlier APPROVED and vice + versa. + """ + latest_by_user: dict[str, dict] = {} + for review in reviews: + if not isinstance(review, dict): + continue + user = (review.get("user") or {}).get("login") + if not isinstance(user, str) or user not in reviewer_set: + continue + state = str(review.get("state") or "").upper() + if state not in {"APPROVED", "REQUEST_CHANGES"}: + continue # ignore COMMENT/PENDING/DISMISSED-state rows + # reviews are returned oldest-first; later entries overwrite → latest wins + latest_by_user[user] = review + + approvers: set[str] = set() + request_changes: list[str] = [] + for user, review in latest_by_user.items(): + if not review.get("official"): + continue + if review.get("stale") or review.get("dismissed"): + continue + commit_id = review.get("commit_id") + if isinstance(commit_id, str) and commit_id and head_sha: + if commit_id != head_sha: + continue # review was on a previous head + state = str(review.get("state") or "").upper() + if state == "APPROVED": + approvers.add(user) + elif state == "REQUEST_CHANGES": + request_changes.append(user) + return approvers, request_changes + + +def get_pull_reviews(pr_number: int) -> list[dict]: + _, body = api("GET", f"/repos/{OWNER}/{NAME}/pulls/{pr_number}/reviews") + if not isinstance(body, list): + raise ApiError(f"PR #{pr_number} reviews response not list") + return body + + def label_names(issue: dict) -> set[str]: return { label["name"] @@ -233,36 +406,87 @@ def pr_has_current_base(pr: dict, commits: list[dict], main_sha: str) -> bool: return pr_contains_base_sha(commits, main_sha) +def _non_required_red_present( + latest: dict[str, dict], + required_contexts: list[str], +) -> bool: + """True if any NON-required context is non-success. + + Such reds are the governance/SOP/advisory checks Gitea may still treat as + "missing required context" at merge time even though branch protection does + not require them. Their presence is what justifies force_merge=true (we + have already verified every REQUIRED context is green and approvals are + genuine, so force only bypasses these non-required reds). + """ + required = set(required_contexts) + for context, status in latest.items(): + if context in required: + continue + if status_state(status) != "success": + return True + return False + + def evaluate_merge_readiness( *, main_status: dict, pr_status: dict, required_contexts: list[str], + required_approvals: int, + approvers: set[str], + request_changes: list[str], pr_has_current_base: bool, + mergeable: bool, pr_labels: set[str] | None = None, ) -> MergeDecision: - # Check push-required contexts explicitly instead of combined state. - # Combined state can be "failure" due to non-blocking jobs - # (continue-on-error: true) that don't actually gate merges. - # CI / all-required (push) is the authoritative gate — it respects - # continue-on-error and correctly aggregates all blocking failures. + # 1) Main's push-required contexts must be green. Combined state can be + # "failure" due to non-blocking jobs (continue-on-error: true) that do + # not gate merges, so check the explicit required set, not combined. main_latest = latest_statuses_by_context(main_status.get("statuses") or []) main_ok, main_bad = required_contexts_green(main_latest, push_required_contexts()) if not main_ok: return MergeDecision(False, "pause", "main required contexts not green: " + ", ".join(main_bad)) + + # 2) PR head must contain current main. if not pr_has_current_base: return MergeDecision(False, "update", "PR head does not contain current main") - # Check explicit required contexts instead of combined state. Combined state - # can be "failure" due to non-blocking jobs with continue-on-error: true - # (e.g. publish-runtime-autobump/pr-validate, qa-review on stale tokens). - # The required_contexts list is the authoritative gate — it includes only - # the checks that actually block merges. + # 3) No open official REQUEST_CHANGES on the current head. + if request_changes: + return MergeDecision( + False, "wait", + "open REQUEST_CHANGES on current head from: " + ", ".join(sorted(request_changes)), + ) + + # 4) Enough distinct genuine official approvals on the current head. + if len(approvers) < required_approvals: + return MergeDecision( + False, "wait", + f"insufficient genuine approvals on current head: have " + f"{len(approvers)} ({', '.join(sorted(approvers)) or 'none'}), " + f"need {required_approvals}", + ) + + # 5) Every BRANCH-PROTECTION-REQUIRED status context must be green. This is + # the authoritative status gate — NON-required reds (qa-review, + # security-review, sop-tier/sop-checklist when not BP-required, E2E Chat, + # Staging SaaS, ci-arm64-advisory, continue-on-error jobs) are NOT + # consulted here and must not block. latest = latest_statuses_by_context(pr_status.get("statuses") or []) ok, missing_or_bad = required_contexts_green(latest, required_contexts, pr_labels) if not ok: return MergeDecision(False, "wait", "required contexts not green: " + ", ".join(missing_or_bad)) - return MergeDecision(True, "merge", "ready") + + # 6) Gitea must consider the PR mergeable (no conflicts). + if not mergeable: + return MergeDecision(False, "wait", "PR is not mergeable (conflicts)") + + # Ready. Use force_merge ONLY if the merge would otherwise be blocked by + # missing-but-non-required governance contexts. Required are green and + # approvals are genuine, so force only bypasses non-required reds — never a + # failing required context or missing approval. + force = _non_required_red_present(latest, required_contexts) + return MergeDecision(True, "merge", "ready", force=force) def get_branch_head(branch: str) -> str: @@ -280,6 +504,12 @@ def get_combined_status(sha: str) -> dict: The /status endpoint caps the `statuses` array at 30 entries (Gitea default page size), so we fetch the full list via /statuses with a higher limit. The combined `state` still comes from /status. + + Fail-closed: the PRIMARY /status fetch must succeed. If it raises, the + error propagates so the caller skips this PR this tick (we never treat a + failed status fetch as green — dev-sop "no fail-open"). Only the SECONDARY + /statuses enrichment (which merely extends the per-context list beyond the + 30-entry cap) is best-effort; if it fails we still have the combined set. """ _, combined = api("GET", f"/repos/{OWNER}/{NAME}/commits/{sha}/status") if not isinstance(combined, dict): @@ -362,22 +592,54 @@ def update_pull(pr_number: int, *, dry_run: bool) -> None: ) -def merge_pull(pr_number: int, *, dry_run: bool) -> None: - payload = { +def add_label_by_name(pr_number: int, label_name: str, *, dry_run: bool) -> None: + """Apply an existing repo label (by name) to a PR/issue. + + Used to HOLD a wedged PR so the queue advances. Resolves the label id from + the repo label set; if the label does not exist, raises ApiError (the + caller decides whether that is fatal). + """ + print(f"::notice::applying label '{label_name}' to PR #{pr_number}") + if dry_run: + return + _, labels = api("GET", f"/repos/{OWNER}/{NAME}/labels", query={"limit": "100"}) + label_id = None + if isinstance(labels, list): + for label in labels: + if isinstance(label, dict) and label.get("name") == label_name: + label_id = label.get("id") + break + if label_id is None: + raise ApiError(f"label '{label_name}' not found in repo {OWNER}/{NAME}") + api( + "POST", + f"/repos/{OWNER}/{NAME}/issues/{pr_number}/labels", + body={"labels": [label_id]}, + ) + + +def merge_pull(pr_number: int, *, dry_run: bool, force: bool = False) -> None: + payload: dict[str, Any] = { "Do": "merge", "MergeTitleField": f"Merge PR #{pr_number} via Gitea merge queue", "MergeMessageField": ( "Serialized merge by gitea-merge-queue after current-main, " - "SOP, and required CI checks were green." + "genuine approvals, and required CI checks were green." ), } - print(f"::notice::merging PR #{pr_number}") + if force: + # force_merge bypasses ONLY missing-but-non-required governance + # contexts. The caller has already verified required contexts are green + # and genuine approvals are present, so this never bypasses a failing + # required context or an approval shortfall. + payload["force_merge"] = True + print(f"::notice::merging PR #{pr_number}{' (force_merge: non-required reds)' if force else ''}") if dry_run: return try: api("POST", f"/repos/{OWNER}/{NAME}/pulls/{pr_number}/merge", body=payload, expect_json=False) except ApiError as exc: - # Re-raise permission-like errors so process_once can skip this PR. + # Re-raise permission-like errors so process_once can HOLD this PR. # 403 = no push access, 404 = repo/pr not found, 405 = not allowed. msg = str(exc) for code in ("403", "404", "405"): @@ -387,7 +649,25 @@ def merge_pull(pr_number: int, *, dry_run: bool) -> None: def process_once(*, dry_run: bool = False) -> int: - contexts = required_contexts(REQUIRED_CONTEXTS_RAW) + # Required status contexts come from BRANCH PROTECTION, not a hand-kept env + # list. Fail-closed: if BP cannot be enumerated, HOLD the whole tick rather + # than merge against an unverified required set. + try: + bp = get_branch_protection(WATCH_BRANCH) + except BranchProtectionUnavailable as exc: + sys.stderr.write( + f"::error::queue held: branch protection for {WATCH_BRANCH} " + f"unavailable (fail-closed): {exc}\n" + ) + return 0 + contexts = bp.required_contexts + required_approvals = bp.required_approvals + print( + f"::notice::queue policy from branch protection: " + f"required_approvals={required_approvals} " + f"required_contexts={contexts or '[none]'}" + ) + main_sha = get_branch_head(WATCH_BRANCH) main_status = get_combined_status(main_sha) # Check push-required contexts explicitly instead of combined state. @@ -424,13 +704,34 @@ def process_once(*, dry_run: bool = False) -> int: raise ApiError(f"PR #{pr_number} missing head sha") commits = get_pull_commits(pr_number) current_base = pr_has_current_base(pr, commits, main_sha) + # Fail-closed: a failed status fetch raises here and the tick is skipped + # (the PR is never treated as green). pr_status = get_combined_status(head_sha) pr_labels = label_names(pr) + # FAIL-CLOSED: Gitea returns mergeable=None (or omits the field) while it is + # still COMPUTING conflict state. Only the literal True is decisive proof the + # PR is conflict-free; None and False both mean "not (yet) mergeable". We must + # NOT autonomously merge on an unknown — treat anything but True as not-yet- + # mergeable so evaluate_merge_readiness returns a transient "wait" decision. + # This is transient: process_once returns 0 (no hold label, no dequeue) and + # the PR is re-checked next tick once Gitea has finished computing mergeability. + mergeable_field = pr.get("mergeable") + mergeable = mergeable_field is True + + reviews = get_pull_reviews(pr_number) + approvers, request_changes = genuine_approvals( + reviews, head_sha=head_sha, reviewer_set=REVIEWER_SET + ) + decision = evaluate_merge_readiness( main_status=main_status, pr_status=pr_status, required_contexts=contexts, + required_approvals=required_approvals, + approvers=approvers, + request_changes=request_changes, pr_has_current_base=current_base, + mergeable=mergeable, pr_labels=pr_labels, ) @@ -455,23 +756,36 @@ def process_once(*, dry_run: bool = False) -> int: ) return 0 try: - merge_pull(pr_number, dry_run=dry_run) + merge_pull(pr_number, dry_run=dry_run, force=decision.force) except MergePermissionError as exc: - # Permanent merge failure (HTTP 403/404/405). Post a comment so - # maintainers know why, then return 0 so this tick is done. - # The PR stays in the queue; future ticks can retry after the - # permission issue is resolved. + # Permanent merge failure (HTTP 403/404/405). This is the + # head-of-line (HOL) bug fix: previously we returned 0 with the PR + # still queued, so the next tick re-selected the SAME wedged PR + # forever and the queue never advanced. Instead, HOLD this PR by + # applying HOLD_LABEL (choose_next_queued_issue skips held PRs), so + # the queue moves on to the next candidate. A maintainer removes + # the hold once the permission issue is fixed. sys.stderr.write(f"::error::merge permission error for PR #{pr_number}: {exc}\n") - post_comment( - pr_number, - ( - "merge-queue: merge failed with HTTP 405 'User not allowed to merge PR'. " - "No available token has Can-merge permission on this repo. " - "Fix: grant Can-merge to a token, or add a maintain/admin collaborator. " - "Skipping to next queued PR on next tick." - ), - dry_run=dry_run, + hold_note = ( + "merge-queue: merge failed with a permanent permission error " + f"({exc}). No available token has Can-merge permission for this " + f"PR. Applied `{HOLD_LABEL}` to unblock the queue (HOL guard). " + f"Fix: grant Can-merge to the queue token, then remove " + f"`{HOLD_LABEL}` to requeue." ) + try: + add_label_by_name(pr_number, HOLD_LABEL, dry_run=dry_run) + except ApiError as label_exc: + # If we cannot even apply the hold label, fall back to a comment + # so the wedge is at least visible; do NOT loop on this PR. + sys.stderr.write( + f"::error::could not apply HOLD_LABEL to PR #{pr_number}: {label_exc}\n" + ) + hold_note += ( + f"\n\n(NOTE: could not apply the hold label automatically: " + f"{label_exc}. Please add `{HOLD_LABEL}` manually.)" + ) + post_comment(pr_number, hold_note, dry_run=dry_run) return 0 return 0 return 0 diff --git a/.gitea/scripts/tests/test_gitea_merge_queue.py b/.gitea/scripts/tests/test_gitea_merge_queue.py index 03dfc2314..440f90dfe 100644 --- a/.gitea/scripts/tests/test_gitea_merge_queue.py +++ b/.gitea/scripts/tests/test_gitea_merge_queue.py @@ -82,38 +82,38 @@ def test_pr_needs_update_when_base_sha_absent_from_commits(): assert mq.pr_contains_base_sha(commits, "parent") is True -def test_merge_decision_requires_main_green_pr_green_and_current_base(): - required = ["CI / all-required (pull_request)"] - main_status = { - "state": "success", - "statuses": [{"context": "CI / all-required (push)", "status": "success"}], - } - pr_status = { - "state": "success", - "statuses": [{"context": "CI / all-required (pull_request)", "status": "success"}], - } - - decision = mq.evaluate_merge_readiness( - main_status=main_status, - pr_status=pr_status, - required_contexts=required, - pr_has_current_base=True, - ) - - assert decision.ready is True - assert decision.action == "merge" - - -def test_merge_decision_updates_stale_pr_before_merge(): - decision = mq.evaluate_merge_readiness( +def _ready_kwargs(**overrides): + """Default kwargs for a fully-ready merge; override per test.""" + base = dict( main_status={ "state": "success", "statuses": [{"context": "CI / all-required (push)", "status": "success"}], }, - pr_status={"state": "success", "statuses": [{"context": "CI / all-required (pull_request)", "status": "success"}]}, + pr_status={ + "state": "success", + "statuses": [{"context": "CI / all-required (pull_request)", "status": "success"}], + }, required_contexts=["CI / all-required (pull_request)"], - pr_has_current_base=False, + required_approvals=2, + approvers={"agent-reviewer-cr2", "agent-researcher"}, + request_changes=[], + pr_has_current_base=True, + mergeable=True, ) + base.update(overrides) + return base + + +def test_merge_decision_requires_main_green_pr_green_and_current_base(): + decision = mq.evaluate_merge_readiness(**_ready_kwargs()) + + assert decision.ready is True + assert decision.action == "merge" + assert decision.force is False # no non-required reds present + + +def test_merge_decision_updates_stale_pr_before_merge(): + decision = mq.evaluate_merge_readiness(**_ready_kwargs(pr_has_current_base=False)) assert decision.ready is False assert decision.action == "update" @@ -127,3 +127,415 @@ def test_MergePermissionError_message_preserved(): exc = mq.MergePermissionError("POST /merge -> HTTP 405: User not allowed") assert "405" in str(exc) assert "User not allowed" in str(exc) + + +# -------------------------------------------------------------------------- +# Fix 1: merge criterion — genuine approvals on current head + required-only +# -------------------------------------------------------------------------- + +REVIEWERS = {"agent-reviewer", "agent-researcher", "agent-reviewer-cr2"} + + +def test_genuine_approvals_counts_two_distinct_on_current_head(): + reviews = [ + {"state": "APPROVED", "user": {"login": "agent-researcher"}, + "official": True, "stale": False, "dismissed": False, "commit_id": "HEAD"}, + {"state": "APPROVED", "user": {"login": "agent-reviewer-cr2"}, + "official": True, "stale": False, "dismissed": False, "commit_id": "HEAD"}, + ] + approvers, rc = mq.genuine_approvals(reviews, head_sha="HEAD", reviewer_set=REVIEWERS) + assert approvers == {"agent-researcher", "agent-reviewer-cr2"} + assert rc == [] + + +def test_genuine_approvals_ignores_stale_dismissed_and_wrong_head(): + reviews = [ + # stale → not counted + {"state": "APPROVED", "user": {"login": "agent-researcher"}, + "official": True, "stale": True, "dismissed": False, "commit_id": "OLD"}, + # dismissed → not counted + {"state": "APPROVED", "user": {"login": "agent-reviewer-cr2"}, + "official": True, "stale": False, "dismissed": True, "commit_id": "HEAD"}, + # commit_id mismatch (prior head) → not counted + {"state": "APPROVED", "user": {"login": "agent-reviewer"}, + "official": True, "stale": False, "dismissed": False, "commit_id": "OLD"}, + ] + approvers, rc = mq.genuine_approvals(reviews, head_sha="HEAD", reviewer_set=REVIEWERS) + assert approvers == set() + assert rc == [] + + +def test_genuine_approvals_ignores_unofficial_and_outsiders(): + reviews = [ + # unofficial → not counted + {"state": "APPROVED", "user": {"login": "agent-researcher"}, + "official": False, "stale": False, "dismissed": False, "commit_id": "HEAD"}, + # outside reviewer set (e.g. CTO-agent / random) → not counted + {"state": "APPROVED", "user": {"login": "hongming-codex-laptop"}, + "official": True, "stale": False, "dismissed": False, "commit_id": "HEAD"}, + ] + approvers, rc = mq.genuine_approvals(reviews, head_sha="HEAD", reviewer_set=REVIEWERS) + assert approvers == set() + + +def test_genuine_approvals_latest_review_supersedes_earlier(): + # agent-reviewer-cr2 approved then later requested changes on same head. + reviews = [ + {"state": "APPROVED", "user": {"login": "agent-reviewer-cr2"}, + "official": True, "stale": False, "dismissed": False, "commit_id": "HEAD"}, + {"state": "REQUEST_CHANGES", "user": {"login": "agent-reviewer-cr2"}, + "official": True, "stale": False, "dismissed": False, "commit_id": "HEAD"}, + ] + approvers, rc = mq.genuine_approvals(reviews, head_sha="HEAD", reviewer_set=REVIEWERS) + assert approvers == set() + assert rc == ["agent-reviewer-cr2"] + + +def test_merge_blocked_when_open_request_changes_on_current_head(): + decision = mq.evaluate_merge_readiness( + **_ready_kwargs(request_changes=["agent-reviewer-cr2"]) + ) + assert decision.ready is False + assert decision.action == "wait" + assert "REQUEST_CHANGES" in decision.reason + + +def test_merge_blocked_when_insufficient_genuine_approvals(): + decision = mq.evaluate_merge_readiness( + **_ready_kwargs(approvers={"agent-researcher"}, required_approvals=2) + ) + assert decision.ready is False + assert decision.action == "wait" + assert "insufficient genuine approvals" in decision.reason + + +def test_non_required_red_does_not_block_merge(): + # Required (CI) green; non-required governance reds present → still merge, + # and force is set so force_merge bypasses ONLY those non-required reds. + pr_status = { + "state": "failure", # combined polluted by non-required reds + "statuses": [ + {"context": "CI / all-required (pull_request)", "status": "success"}, + {"context": "qa-review / approved (pull_request)", "status": "failure"}, + {"context": "security-review / approved (pull_request)", "status": "pending"}, + {"context": "sop-tier-check / tier-check (pull_request)", "status": "failure"}, + {"context": "Staging SaaS / e2e (pull_request)", "status": "failure"}, + ], + } + decision = mq.evaluate_merge_readiness(**_ready_kwargs(pr_status=pr_status)) + assert decision.ready is True + assert decision.action == "merge" + assert decision.force is True + + +def test_failing_required_context_blocks_even_with_approvals(): + pr_status = { + "state": "failure", + "statuses": [ + {"context": "CI / all-required (pull_request)", "status": "failure"}, + ], + } + decision = mq.evaluate_merge_readiness(**_ready_kwargs(pr_status=pr_status)) + assert decision.ready is False + assert decision.action == "wait" + assert decision.force is False + assert "required contexts not green" in decision.reason + + +def test_unmergeable_pr_blocks(): + decision = mq.evaluate_merge_readiness(**_ready_kwargs(mergeable=False)) + assert decision.ready is False + assert decision.action == "wait" + assert "not mergeable" in decision.reason + + +# -------------------------------------------------------------------------- +# Fix 1 (cont.): required contexts come from branch protection (fail-closed) +# -------------------------------------------------------------------------- + +def test_parse_branch_protection_uses_status_check_contexts(): + bp = mq.parse_branch_protection({ + "enable_status_check": True, + "status_check_contexts": [ + "CI / all-required (pull_request)", + "E2E API Smoke Test / E2E API Smoke Test (pull_request)", + ], + "required_approvals": 2, + "block_on_rejected_reviews": True, + }) + assert bp.required_contexts == [ + "CI / all-required (pull_request)", + "E2E API Smoke Test / E2E API Smoke Test (pull_request)", + ] + assert bp.required_approvals == 2 + assert bp.block_on_rejected_reviews is True + + +def test_parse_branch_protection_fail_closed_when_contexts_missing(): + # enable_status_check true but no enumerable contexts → must raise so the + # queue HOLDS rather than merging against an unverified required set. + import pytest + with pytest.raises(mq.BranchProtectionUnavailable): + mq.parse_branch_protection({ + "enable_status_check": True, + "status_check_contexts": None, + "required_approvals": 2, + }) + with pytest.raises(mq.BranchProtectionUnavailable): + mq.parse_branch_protection({ + "enable_status_check": True, + "status_check_contexts": [], + "required_approvals": 2, + }) + + +def test_parse_branch_protection_fail_closed_on_non_object(): + import pytest + with pytest.raises(mq.BranchProtectionUnavailable): + mq.parse_branch_protection(None) + + +# -------------------------------------------------------------------------- +# Fix 2: HOL — a permanent merge error HOLDS the PR (applies HOLD_LABEL) +# -------------------------------------------------------------------------- + +def test_process_once_holds_pr_on_permanent_merge_error(monkeypatch): + """A 405 on merge must apply HOLD_LABEL so the queue advances, not loop.""" + calls = {"hold_label": None, "merge_attempts": 0} + + monkeypatch.setattr(mq, "OWNER", "molecule-ai") + monkeypatch.setattr(mq, "NAME", "molecule-core") + monkeypatch.setattr(mq, "WATCH_BRANCH", "main") + monkeypatch.setattr(mq, "QUEUE_LABEL", "merge-queue") + monkeypatch.setattr(mq, "HOLD_LABEL", "merge-queue-hold") + monkeypatch.setattr(mq, "REVIEWER_SET", REVIEWERS) + + monkeypatch.setattr(mq, "get_branch_protection", lambda branch: mq.BranchProtection( + required_contexts=["CI / all-required (pull_request)"], + required_approvals=2, + block_on_rejected_reviews=True, + )) + main_sha = "b" * 40 + head_sha = "a" * 40 + monkeypatch.setattr(mq, "get_branch_head", lambda branch: main_sha) + + def fake_combined(sha): + ctx = "CI / all-required (push)" if sha == main_sha else "CI / all-required (pull_request)" + return {"state": "success", "statuses": [{"context": ctx, "status": "success"}]} + monkeypatch.setattr(mq, "get_combined_status", fake_combined) + + monkeypatch.setattr(mq, "list_queued_issues", lambda: [ + {"number": 100, "pull_request": {}, "labels": [{"name": "merge-queue"}], + "created_at": "2026-06-01T00:00:00Z"}, + ]) + monkeypatch.setattr(mq, "get_pull", lambda n: { + "state": "open", "number": n, "mergeable": True, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": head_sha, "repo_id": 1}, + "labels": [{"name": "merge-queue"}], + }) + monkeypatch.setattr(mq, "get_pull_commits", lambda n: [{"sha": main_sha}, {"sha": head_sha}]) + monkeypatch.setattr(mq, "get_pull_reviews", lambda n: [ + {"state": "APPROVED", "user": {"login": "agent-researcher"}, + "official": True, "stale": False, "dismissed": False, "commit_id": head_sha}, + {"state": "APPROVED", "user": {"login": "agent-reviewer-cr2"}, + "official": True, "stale": False, "dismissed": False, "commit_id": head_sha}, + ]) + + def fake_merge(pr_number, *, dry_run, force=False): + calls["merge_attempts"] += 1 + raise mq.MergePermissionError("POST /merge -> HTTP 405: User not allowed to merge PR") + monkeypatch.setattr(mq, "merge_pull", fake_merge) + + def fake_add_label(pr_number, label_name, *, dry_run): + calls["hold_label"] = (pr_number, label_name) + monkeypatch.setattr(mq, "add_label_by_name", fake_add_label) + monkeypatch.setattr(mq, "post_comment", lambda *a, **k: None) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + assert calls["merge_attempts"] == 1 + # The HOL fix: PR was held, not silently left re-selectable. + assert calls["hold_label"] == (100, "merge-queue-hold") + + +# -------------------------------------------------------------------------- +# Fix 2 (cont.): mergeable=None is fail-CLOSED — Gitea is still computing the +# conflict check, so the queue must WAIT (re-check next tick), NOT merge. Only +# an explicit mergeable==True proceeds to an autonomous merge. +# -------------------------------------------------------------------------- + +def _fully_ready_process_once_monkeypatch(monkeypatch, mergeable, calls): + """Wire process_once so every gate is green EXCEPT the `mergeable` field, + which is set to the value under test. Records merge attempts in `calls`.""" + monkeypatch.setattr(mq, "OWNER", "molecule-ai") + monkeypatch.setattr(mq, "NAME", "molecule-core") + monkeypatch.setattr(mq, "WATCH_BRANCH", "main") + monkeypatch.setattr(mq, "QUEUE_LABEL", "merge-queue") + monkeypatch.setattr(mq, "HOLD_LABEL", "merge-queue-hold") + monkeypatch.setattr(mq, "REVIEWER_SET", REVIEWERS) + monkeypatch.setattr(mq, "get_branch_protection", lambda branch: mq.BranchProtection( + required_contexts=["CI / all-required (pull_request)"], + required_approvals=2, + block_on_rejected_reviews=True, + )) + main_sha = "b" * 40 + head_sha = "a" * 40 + monkeypatch.setattr(mq, "get_branch_head", lambda branch: main_sha) + + def fake_combined(sha): + ctx = "CI / all-required (push)" if sha == main_sha else "CI / all-required (pull_request)" + return {"state": "success", "statuses": [{"context": ctx, "status": "success"}]} + monkeypatch.setattr(mq, "get_combined_status", fake_combined) + + monkeypatch.setattr(mq, "list_queued_issues", lambda: [ + {"number": 102, "pull_request": {}, "labels": [{"name": "merge-queue"}], + "created_at": "2026-06-01T00:00:00Z"}, + ]) + # mergeable is the value under test; everything else is fully ready. + monkeypatch.setattr(mq, "get_pull", lambda n: { + "state": "open", "number": n, "mergeable": mergeable, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": head_sha, "repo_id": 1}, + "labels": [{"name": "merge-queue"}], + }) + monkeypatch.setattr(mq, "get_pull_commits", lambda n: [{"sha": main_sha}, {"sha": head_sha}]) + monkeypatch.setattr(mq, "get_pull_reviews", lambda n: [ + {"state": "APPROVED", "user": {"login": "agent-researcher"}, + "official": True, "stale": False, "dismissed": False, "commit_id": head_sha}, + {"state": "APPROVED", "user": {"login": "agent-reviewer-cr2"}, + "official": True, "stale": False, "dismissed": False, "commit_id": head_sha}, + ]) + + def fake_merge(pr_number, *, dry_run, force=False): + calls["merge_attempts"] += 1 + monkeypatch.setattr(mq, "merge_pull", fake_merge) + + def fake_add_label(pr_number, label_name, *, dry_run): + calls["hold_label"] = (pr_number, label_name) + monkeypatch.setattr(mq, "add_label_by_name", fake_add_label) + monkeypatch.setattr(mq, "update_pull", lambda *a, **k: calls.__setitem__("updated", True)) + monkeypatch.setattr(mq, "post_comment", lambda *a, **k: None) + + +def test_process_once_waits_when_mergeable_is_none(monkeypatch): + """FAIL-CLOSED: mergeable=None means Gitea is still computing the conflict + check. The queue must NOT merge this tick; it waits and re-checks next tick. + Critically: this is transient, so the PR is NOT hold-labelled (it stays + queued and re-selectable).""" + calls = {"merge_attempts": 0, "hold_label": None, "updated": False} + _fully_ready_process_once_monkeypatch(monkeypatch, mergeable=None, calls=calls) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + # The bug regression: a None mergeable must NEVER trigger an autonomous merge. + assert calls["merge_attempts"] == 0 + # Transient, not permanent — must NOT be held/dequeued; retried next tick. + assert calls["hold_label"] is None + assert calls["updated"] is False + + +def test_process_once_waits_when_mergeable_field_absent(monkeypatch): + """Some Gitea versions omit the `mergeable` field entirely. Absent must be + treated the same as None — fail-closed, wait, no merge.""" + calls = {"merge_attempts": 0, "hold_label": None, "updated": False} + # Reuse the ready wiring but drop the mergeable key from the pull payload. + _fully_ready_process_once_monkeypatch(monkeypatch, mergeable=None, calls=calls) + head_sha = "a" * 40 + monkeypatch.setattr(mq, "get_pull", lambda n: { + "state": "open", "number": n, # no "mergeable" key at all + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": head_sha, "repo_id": 1}, + "labels": [{"name": "merge-queue"}], + }) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + assert calls["merge_attempts"] == 0 + assert calls["hold_label"] is None + + +def test_process_once_merges_when_mergeable_is_true(monkeypatch): + """The decisive case: an explicit mergeable==True (with every other gate + green) DOES proceed to the autonomous merge.""" + calls = {"merge_attempts": 0, "hold_label": None, "updated": False} + _fully_ready_process_once_monkeypatch(monkeypatch, mergeable=True, calls=calls) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + assert calls["merge_attempts"] == 1 + assert calls["hold_label"] is None + + +# -------------------------------------------------------------------------- +# Fix 3: status fetch is fail-closed (failed fetch != green) +# -------------------------------------------------------------------------- + +def test_status_fetch_failure_is_fail_closed(monkeypatch): + """If the PR head status fetch raises, the PR is skipped — never merged.""" + merged = {"called": False} + + monkeypatch.setattr(mq, "OWNER", "molecule-ai") + monkeypatch.setattr(mq, "NAME", "molecule-core") + monkeypatch.setattr(mq, "WATCH_BRANCH", "main") + monkeypatch.setattr(mq, "QUEUE_LABEL", "merge-queue") + monkeypatch.setattr(mq, "HOLD_LABEL", "merge-queue-hold") + monkeypatch.setattr(mq, "REVIEWER_SET", REVIEWERS) + monkeypatch.setattr(mq, "get_branch_protection", lambda branch: mq.BranchProtection( + required_contexts=["CI / all-required (pull_request)"], + required_approvals=2, block_on_rejected_reviews=True, + )) + main_sha = "b" * 40 + head_sha = "a" * 40 + monkeypatch.setattr(mq, "get_branch_head", lambda branch: main_sha) + + def fake_combined(sha): + if sha == main_sha: + return {"state": "success", + "statuses": [{"context": "CI / all-required (push)", "status": "success"}]} + # PR head status fetch fails — fail-closed must propagate. + raise mq.ApiError("GET /commits/HEAD/status -> HTTP 502: bad gateway") + monkeypatch.setattr(mq, "get_combined_status", fake_combined) + + monkeypatch.setattr(mq, "list_queued_issues", lambda: [ + {"number": 101, "pull_request": {}, "labels": [{"name": "merge-queue"}], + "created_at": "2026-06-01T00:00:00Z"}, + ]) + monkeypatch.setattr(mq, "get_pull", lambda n: { + "state": "open", "number": n, "mergeable": True, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": head_sha, "repo_id": 1}, + "labels": [{"name": "merge-queue"}], + }) + monkeypatch.setattr(mq, "get_pull_commits", lambda n: [{"sha": main_sha}, {"sha": head_sha}]) + monkeypatch.setattr(mq, "get_pull_reviews", lambda n: []) + + def fake_merge(*a, **k): + merged["called"] = True + monkeypatch.setattr(mq, "merge_pull", fake_merge) + + # process_once lets the ApiError propagate; main() swallows it as a tick + # no-op. Either way, no merge happens. + import pytest + with pytest.raises(mq.ApiError): + mq.process_once(dry_run=False) + assert merged["called"] is False + + +def test_process_once_holds_tick_when_branch_protection_unavailable(monkeypatch): + """BP enumeration failure → HOLD the whole tick (no merge, rc 0).""" + merged = {"called": False} + monkeypatch.setattr(mq, "WATCH_BRANCH", "main") + + def fake_bp(branch): + raise mq.BranchProtectionUnavailable("403 forbidden") + monkeypatch.setattr(mq, "get_branch_protection", fake_bp) + monkeypatch.setattr(mq, "merge_pull", lambda *a, **k: merged.__setitem__("called", True)) + + rc = mq.process_once(dry_run=False) + assert rc == 0 + assert merged["called"] is False diff --git a/.gitea/workflows/gitea-merge-queue.yml b/.gitea/workflows/gitea-merge-queue.yml index c8628f37b..f5752a243 100644 --- a/.gitea/workflows/gitea-merge-queue.yml +++ b/.gitea/workflows/gitea-merge-queue.yml @@ -49,9 +49,20 @@ jobs: QUEUE_LABEL: merge-queue HOLD_LABEL: merge-queue-hold UPDATE_STYLE: merge - REQUIRED_CONTEXTS: >- - CI / all-required (pull_request), - sop-checklist / all-items-acked (pull_request) + # Recognised official-reviewer set. A merge needs >= required_approvals + # DISTINCT genuine official approvals from these accounts on the + # CURRENT head sha (not stale/dismissed). The required_approvals count + # itself is read from branch protection at runtime. + REVIEWER_SET: agent-reviewer,agent-researcher,agent-reviewer-cr2 + # NOTE: REQUIRED_CONTEXTS is no longer the authoritative PR gate. The + # queue now reads the required status contexts from BRANCH PROTECTION + # (status_check_contexts) so non-required governance reds (qa-review, + # security-review, sop-tier, sop-checklist when not branch-required, + # E2E Chat, Staging SaaS, ci-arm64-advisory) cannot block a merge. + # If branch protection cannot be enumerated the queue HOLDS + # (fail-closed). REQUIRED_APPROVALS below is only a fallback used when + # branch protection does not specify required_approvals. + REQUIRED_APPROVALS: "2" # Push-side required contexts. Checking CI / all-required (push) # explicitly instead of the combined state avoids false-pause when # non-blocking jobs (continue-on-error: true) have failed — those