From 2fa68b1f23f5cfe02f4aec517f55a3a404fd8240 Mon Sep 17 00:00:00 2001 From: devops-engineer Date: Sat, 6 Jun 2026 01:53:31 -0700 Subject: [PATCH 1/2] merge-queue: auto-discovery (opt-OUT, label-optional) for self-sustaining autonomy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The external Gitea merge queue only considered PRs that already carried the `merge-queue` label. Agent Gitea tokens lack `write:issue` (labels are issue-scoped), so agents could never self-label a ready PR — the queue stalled waiting on a human to add the label, blocking core-PR autonomy (#2355). Fix: merge-on-criteria, label-optional. The cron now AUTO-DISCOVERS every open same-repo PR and considers any that meets the unchanged merge bar. The `merge-queue` label is now optional metadata, not a gate — this fully removes the write:issue dependency (the cron itself never needs to add a label). SAFETY is preserved as opt-OUT: a PR carrying any opt-out label (`merge-queue-hold`, `do-not-auto-merge`, or `wip`) or marked draft is skipped (never auto-considered, never merged). A human keeps a PR out of autonomous merging by adding one of those labels. `AUTO_DISCOVER=0` restores legacy opt-IN. The merge bar is UNCHANGED: still 2 genuine official approvals on the CURRENT head from {agent-reviewer, agent-researcher, agent-reviewer-cr2}, all branch-protection-required contexts green, mergeable=True (fail-closed on None/False per #2349/#2352), and no open REQUEST_CHANGES. Auto-discovery only changes WHICH PRs are considered, not whether they may merge. - new `do-not-auto-merge` (id 78) + `wip` (id 79) repo labels - `choose_next_candidate_issue` / `list_candidate_issues` for the opt-OUT, draft-skipping selection; legacy `choose_next_queued_issue` retained - defensive opt-out/draft re-check on the live pull payload (stale-listing race) - 15 new §SOP-22 regression tests; existing 26 kept green (41 total) - workflow + runbook updated (AUTO_DISCOVER / OPT_OUT_LABELS documented) Verified live (dry-run): auto-discovery selects unlabeled PR #1519 (the old code never touched it); AUTO_DISCOVER=0 still selects only labeled #2346. Helps #2355 (autonomy expansion). Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitea/scripts/gitea-merge-queue.py | 146 +++++++- .../scripts/tests/test_gitea_merge_queue.py | 332 +++++++++++++++++- .gitea/workflows/gitea-merge-queue.yml | 22 +- runbooks/gitea-merge-queue.md | 44 ++- 4 files changed, 519 insertions(+), 25 deletions(-) diff --git a/.gitea/scripts/gitea-merge-queue.py b/.gitea/scripts/gitea-merge-queue.py index 5b494d7bd..18a41d68a 100644 --- a/.gitea/scripts/gitea-merge-queue.py +++ b/.gitea/scripts/gitea-merge-queue.py @@ -4,7 +4,8 @@ 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 (skipping HOLD_LABEL). +1. Pick the oldest open same-repo PR that is NOT opted out (auto-discovery, + see below), skipping drafts. 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 @@ -29,6 +30,26 @@ Authoritative gates (fail-closed): approvals present). It is NEVER used to bypass a failing REQUIRED context or missing approvals. +Auto-discovery (opt-OUT, label-optional): + The queue is SELF-SUSTAINING — a ready PR does NOT need a human (or an agent) + to add the `merge-queue` label first. When AUTO_DISCOVER is on (default), the + queue enumerates ALL open same-repo PRs and considers any that meets the full + merge bar (genuine approvals on current head + BP-required green + mergeable + + no open REQUEST_CHANGES). The merge bar above is UNCHANGED; auto-discovery only + changes WHICH PRs are considered, not whether they are mergeable. + + This deliberately removes the historical dependency on an agent adding the + `merge-queue` label — agent Gitea tokens lack `write:issue` (labels are + issue-scoped), so they could never self-label and the queue stalled. The label + is now OPTIONAL metadata, not a gate. + + SAFETY is preserved as opt-OUT: any PR carrying an opt-out label + (OPT_OUT_LABELS — `merge-queue-hold`, `do-not-auto-merge`, `wip` by default) is + skipped (never auto-considered, never merged). Draft PRs (draft=true) are also + skipped. A human who wants to keep a PR out of autonomous merging just adds one + of those labels. Setting AUTO_DISCOVER=0 restores the legacy opt-IN behaviour + (only PRs already carrying QUEUE_LABEL are considered). + 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. Likewise, a @@ -68,6 +89,30 @@ WATCH_BRANCH = _env("WATCH_BRANCH", default="main") QUEUE_LABEL = _env("QUEUE_LABEL", default="merge-queue") HOLD_LABEL = _env("HOLD_LABEL", default="merge-queue-hold") UPDATE_STYLE = _env("UPDATE_STYLE", default="merge") +# Auto-discovery (opt-OUT). When truthy (default), the queue considers ALL open +# same-repo PRs that meet the merge bar, not only PRs already carrying +# QUEUE_LABEL — so the queue is self-sustaining without any human/agent labeling +# (agent tokens lack write:issue and cannot self-label). Set AUTO_DISCOVER=0 to +# restore the legacy opt-IN behaviour (QUEUE_LABEL required to be considered). +AUTO_DISCOVER = _env("AUTO_DISCOVER", default="1").strip().lower() not in { + "0", + "false", + "no", + "off", + "", +} +# Opt-OUT labels. A PR carrying ANY of these is skipped (never auto-considered, +# never merged) — the human escape hatch from autonomous merging. HOLD_LABEL is +# always included so the existing hold semantics keep working. `do-not-auto-merge` +# and `wip` let a human keep a PR out of the auto-merge path without removing it. +OPT_OUT_LABELS = { + name.strip() + for name in _env( + "OPT_OUT_LABELS", + default="do-not-auto-merge,wip", + ).split(",") + if name.strip() +} | ({HOLD_LABEL} if HOLD_LABEL else set()) REQUIRED_CONTEXTS_RAW = _env( "REQUIRED_CONTEXTS", default=( @@ -410,6 +455,58 @@ def choose_next_queued_issue( return candidates[0] if candidates else None +def _issue_is_draft(issue: dict) -> bool: + """True if the issue/PR is a draft. + + The /issues listing exposes draft state under the `pull_request` sub-object + (`{"draft": true}`); some Gitea versions also surface a top-level `draft`. + Either is honoured. Drafts are never auto-considered for merging. + """ + pr = issue.get("pull_request") + if isinstance(pr, dict) and pr.get("draft") is True: + return True + return issue.get("draft") is True + + +def choose_next_candidate_issue( + issues: list[dict], + *, + queue_label: str, + opt_out_labels: set[str], + auto_discover: bool, +) -> dict | None: + """Pick the oldest open PR eligible for a merge attempt this tick. + + This is the auto-discovery selector. It does NOT change the merge bar — it + only changes WHICH PRs are considered: + + - auto_discover=True (default): every open same-repo PR is a candidate, + EXCEPT those carrying an opt-out label or marked draft. The QUEUE_LABEL + is optional metadata, not a gate, so a ready PR reaches the queue with no + human/agent labeling (the write:issue gap is removed). + - auto_discover=False: legacy opt-IN — only PRs carrying queue_label are + candidates (still skipping opt-out labels and drafts). + + Opt-out is the safety escape hatch: any opt_out_labels member present skips + the PR entirely (never considered, never merged). Selection is oldest-first + (created_at, then number) to preserve the serialized FIFO ordering. + """ + candidates = [] + for issue in issues: + if "pull_request" not in issue: + continue + labels = label_names(issue) + if opt_out_labels & labels: + continue # opt-out: human kept this PR out of autonomous merging + if _issue_is_draft(issue): + continue # drafts are never auto-merged + if not auto_discover and queue_label not in labels: + continue # legacy opt-IN: require the queue label + candidates.append(issue) + candidates.sort(key=lambda issue: (issue.get("created_at") or "", int(issue["number"]))) + return candidates[0] if candidates else None + + def pr_contains_base_sha(commits: list[dict], base_sha: str) -> bool: for commit in commits: sha = commit.get("sha") or commit.get("id") @@ -577,6 +674,31 @@ def list_queued_issues() -> list[dict]: return body +def list_candidate_issues(*, auto_discover: bool) -> list[dict]: + """Open PR issues eligible for consideration this tick. + + With auto_discover=True (default) this enumerates ALL open PRs (no label + filter) so the queue is self-sustaining — a ready PR is considered without + any human/agent first adding QUEUE_LABEL. With auto_discover=False it falls + back to the legacy label-filtered listing (opt-IN). Opt-out filtering and + draft-skipping happen in choose_next_candidate_issue, not here. + """ + if not auto_discover: + return list_queued_issues() + _, body = api( + "GET", + f"/repos/{OWNER}/{NAME}/issues", + query={ + "state": "open", + "type": "pulls", + "limit": "50", + }, + ) + if not isinstance(body, list): + raise ApiError("candidate issues response not list") + return body + + def get_pull(pr_number: int) -> dict: _, body = api("GET", f"/repos/{OWNER}/{NAME}/pulls/{pr_number}") if not isinstance(body, dict): @@ -731,13 +853,17 @@ def process_once(*, dry_run: bool = False) -> int: print(f"::notice::queue paused: {WATCH_BRANCH}@{main_sha[:8]} required contexts not green: {', '.join(main_bad)}") return 0 - issue = choose_next_queued_issue( - list_queued_issues(), + issue = choose_next_candidate_issue( + list_candidate_issues(auto_discover=AUTO_DISCOVER), queue_label=QUEUE_LABEL, - hold_label=HOLD_LABEL, + opt_out_labels=OPT_OUT_LABELS, + auto_discover=AUTO_DISCOVER, ) if not issue: - print("::notice::merge queue empty") + print( + "::notice::no merge candidates " + f"(auto_discover={'on' if AUTO_DISCOVER else 'off'})" + ) return 0 pr_number = int(issue["number"]) @@ -745,6 +871,16 @@ def process_once(*, dry_run: bool = False) -> int: if pr.get("state") != "open": print(f"::notice::PR #{pr_number} is not open; skipping") return 0 + # Defensive opt-out/draft re-check on the authoritative pull payload: the + # /issues listing's label/draft view can lag, but the merge bar must respect + # the live pull state. (choose_next_candidate_issue already filtered on the + # listing; this guards against a stale listing racing a just-added opt-out.) + if OPT_OUT_LABELS & label_names(pr): + print(f"::notice::PR #{pr_number} carries an opt-out label; skipping") + return 0 + if pr.get("draft") is True: + print(f"::notice::PR #{pr_number} is a draft; skipping") + return 0 if pr.get("base", {}).get("ref") != WATCH_BRANCH: post_comment(pr_number, f"merge-queue: skipped; base branch is not `{WATCH_BRANCH}`.", dry_run=dry_run) return 0 diff --git a/.gitea/scripts/tests/test_gitea_merge_queue.py b/.gitea/scripts/tests/test_gitea_merge_queue.py index ccac256f6..c2366ec55 100644 --- a/.gitea/scripts/tests/test_gitea_merge_queue.py +++ b/.gitea/scripts/tests/test_gitea_merge_queue.py @@ -308,6 +308,8 @@ def test_process_once_holds_pr_on_permanent_merge_error(monkeypatch): monkeypatch.setattr(mq, "WATCH_BRANCH", "main") monkeypatch.setattr(mq, "QUEUE_LABEL", "merge-queue") monkeypatch.setattr(mq, "HOLD_LABEL", "merge-queue-hold") + monkeypatch.setattr(mq, "AUTO_DISCOVER", True) + monkeypatch.setattr(mq, "OPT_OUT_LABELS", {"merge-queue-hold", "do-not-auto-merge", "wip"}) monkeypatch.setattr(mq, "REVIEWER_SET", REVIEWERS) monkeypatch.setattr(mq, "get_branch_protection", lambda branch: mq.BranchProtection( @@ -324,7 +326,7 @@ def test_process_once_holds_pr_on_permanent_merge_error(monkeypatch): return {"state": "success", "statuses": [{"context": ctx, "status": "success"}]} monkeypatch.setattr(mq, "get_combined_status", fake_combined) - monkeypatch.setattr(mq, "list_queued_issues", lambda: [ + monkeypatch.setattr(mq, "list_candidate_issues", lambda *, auto_discover: [ {"number": 100, "pull_request": {}, "labels": [{"name": "merge-queue"}], "created_at": "2026-06-01T00:00:00Z"}, ]) @@ -374,6 +376,8 @@ def _fully_ready_process_once_monkeypatch(monkeypatch, mergeable, calls): monkeypatch.setattr(mq, "WATCH_BRANCH", "main") monkeypatch.setattr(mq, "QUEUE_LABEL", "merge-queue") monkeypatch.setattr(mq, "HOLD_LABEL", "merge-queue-hold") + monkeypatch.setattr(mq, "AUTO_DISCOVER", True) + monkeypatch.setattr(mq, "OPT_OUT_LABELS", {"merge-queue-hold", "do-not-auto-merge", "wip"}) monkeypatch.setattr(mq, "REVIEWER_SET", REVIEWERS) monkeypatch.setattr(mq, "get_branch_protection", lambda branch: mq.BranchProtection( required_contexts=["CI / all-required (pull_request)"], @@ -389,7 +393,7 @@ def _fully_ready_process_once_monkeypatch(monkeypatch, mergeable, calls): return {"state": "success", "statuses": [{"context": ctx, "status": "success"}]} monkeypatch.setattr(mq, "get_combined_status", fake_combined) - monkeypatch.setattr(mq, "list_queued_issues", lambda: [ + monkeypatch.setattr(mq, "list_candidate_issues", lambda *, auto_discover: [ {"number": 102, "pull_request": {}, "labels": [{"name": "merge-queue"}], "created_at": "2026-06-01T00:00:00Z"}, ]) @@ -484,6 +488,8 @@ def test_status_fetch_failure_is_fail_closed(monkeypatch): monkeypatch.setattr(mq, "WATCH_BRANCH", "main") monkeypatch.setattr(mq, "QUEUE_LABEL", "merge-queue") monkeypatch.setattr(mq, "HOLD_LABEL", "merge-queue-hold") + monkeypatch.setattr(mq, "AUTO_DISCOVER", True) + monkeypatch.setattr(mq, "OPT_OUT_LABELS", {"merge-queue-hold", "do-not-auto-merge", "wip"}) monkeypatch.setattr(mq, "REVIEWER_SET", REVIEWERS) monkeypatch.setattr(mq, "get_branch_protection", lambda branch: mq.BranchProtection( required_contexts=["CI / all-required (pull_request)"], @@ -501,7 +507,7 @@ def test_status_fetch_failure_is_fail_closed(monkeypatch): 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: [ + monkeypatch.setattr(mq, "list_candidate_issues", lambda *, auto_discover: [ {"number": 101, "pull_request": {}, "labels": [{"name": "merge-queue"}], "created_at": "2026-06-01T00:00:00Z"}, ]) @@ -702,3 +708,323 @@ def test_queue_advances_past_held_conflicted_pr(monkeypatch): ) assert selected is not None assert selected["number"] == 1500 + + +# -------------------------------------------------------------------------- +# §SOP-22: AUTO-DISCOVERY (opt-OUT, label-optional). The queue must be +# self-sustaining — a ready PR is considered/merged with NO `merge-queue` +# label, while opt-out labels (merge-queue-hold / do-not-auto-merge / wip) and +# drafts are skipped. The merge bar (approvals/required-green/mergeable) is +# unchanged; only candidate selection changes. +# -------------------------------------------------------------------------- + +OPT_OUT = {"merge-queue-hold", "do-not-auto-merge", "wip"} + + +def _issue(number, labels, *, created="2026-06-01T00:00:00Z", draft=False, is_pr=True): + pr = {"draft": draft} if is_pr else None + out = { + "number": number, + "labels": [{"name": n} for n in labels], + "created_at": created, + } + if pr is not None: + out["pull_request"] = pr + return out + + +def test_auto_discover_selects_unlabeled_ready_pr(): + """A ready PR with NO merge-queue label is auto-considered (the autonomy fix: + agents cannot self-label because their token lacks write:issue).""" + issues = [_issue(50, labels=[])] # no merge-queue label at all + selected = mq.choose_next_candidate_issue( + issues, queue_label="merge-queue", opt_out_labels=OPT_OUT, auto_discover=True + ) + assert selected is not None + assert selected["number"] == 50 + + +def test_auto_discover_skips_opt_out_labels(): + """Each opt-out label keeps a PR OUT of autonomous merging (the human escape + hatch). A PR carrying any of them is never selected even though it is open.""" + for optout in OPT_OUT: + issues = [_issue(60, labels=[optout])] + selected = mq.choose_next_candidate_issue( + issues, queue_label="merge-queue", opt_out_labels=OPT_OUT, auto_discover=True + ) + assert selected is None, f"{optout!r} should opt the PR out" + + +def test_auto_discover_skips_opt_out_even_when_queue_labeled(): + """An opt-out label beats the merge-queue label: a held/wip PR that also + carries merge-queue is still skipped.""" + issues = [_issue(61, labels=["merge-queue", "wip"])] + selected = mq.choose_next_candidate_issue( + issues, queue_label="merge-queue", opt_out_labels=OPT_OUT, auto_discover=True + ) + assert selected is None + + +def test_auto_discover_skips_drafts(): + issues = [_issue(62, labels=[], draft=True)] + selected = mq.choose_next_candidate_issue( + issues, queue_label="merge-queue", opt_out_labels=OPT_OUT, auto_discover=True + ) + assert selected is None + + +def test_auto_discover_skips_non_pull_issues(): + """A plain issue (no pull_request key) is never a merge candidate.""" + issues = [_issue(63, labels=[], is_pr=False)] + selected = mq.choose_next_candidate_issue( + issues, queue_label="merge-queue", opt_out_labels=OPT_OUT, auto_discover=True + ) + assert selected is None + + +def test_auto_discover_oldest_first_skipping_opt_out(): + """Selection is FIFO (oldest created_at first), and the opt-out PR is passed + over for the next-oldest eligible PR.""" + issues = [ + _issue(70, labels=["do-not-auto-merge"], created="2026-06-01T01:00:00Z"), + _issue(71, labels=[], created="2026-06-01T02:00:00Z"), + _issue(72, labels=["merge-queue"], created="2026-06-01T03:00:00Z"), + ] + selected = mq.choose_next_candidate_issue( + issues, queue_label="merge-queue", opt_out_labels=OPT_OUT, auto_discover=True + ) + assert selected["number"] == 71 # 70 opted out, 71 is next-oldest eligible + + +def test_opt_in_mode_requires_queue_label(): + """AUTO_DISCOVER off restores legacy opt-IN: only merge-queue-labeled PRs are + candidates; an unlabeled ready PR is NOT selected.""" + issues = [ + _issue(80, labels=[], created="2026-06-01T01:00:00Z"), + _issue(81, labels=["merge-queue"], created="2026-06-01T02:00:00Z"), + ] + selected = mq.choose_next_candidate_issue( + issues, queue_label="merge-queue", opt_out_labels=OPT_OUT, auto_discover=False + ) + assert selected["number"] == 81 + + +def test_opt_in_mode_still_honours_opt_out(): + """Even in opt-IN mode, an opt-out label on a queue-labeled PR skips it.""" + issues = [_issue(82, labels=["merge-queue", "merge-queue-hold"])] + selected = mq.choose_next_candidate_issue( + issues, queue_label="merge-queue", opt_out_labels=OPT_OUT, auto_discover=False + ) + assert selected is None + + +def test_list_candidate_issues_omits_label_filter_when_auto_discover(monkeypatch): + """The auto-discovery listing must NOT pass a `labels` filter (so unlabeled + PRs are enumerated); the opt-IN listing must keep filtering by QUEUE_LABEL.""" + captured = {} + + def fake_api(method, path, *, query=None, **kw): + captured["query"] = dict(query or {}) + return 200, [] + + monkeypatch.setattr(mq, "api", fake_api) + monkeypatch.setattr(mq, "QUEUE_LABEL", "merge-queue") + + mq.list_candidate_issues(auto_discover=True) + assert "labels" not in captured["query"] + assert captured["query"].get("type") == "pulls" + + mq.list_candidate_issues(auto_discover=False) + assert captured["query"].get("labels") == "merge-queue" + + +def _wire_ready_process_once(monkeypatch, *, issues, pr_payload, calls): + """Wire process_once fully green EXCEPT candidate selection / pull payload, + which the caller supplies to exercise auto-discovery end-to-end.""" + 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, "AUTO_DISCOVER", True) + monkeypatch.setattr(mq, "OPT_OUT_LABELS", OPT_OUT) + 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_candidate_issues", lambda *, auto_discover: issues) + monkeypatch.setattr(mq, "get_pull", lambda n: dict(pr_payload, number=n)) + 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["merged"] = pr_number + monkeypatch.setattr(mq, "merge_pull", fake_merge) + monkeypatch.setattr(mq, "update_pull", lambda *a, **k: calls.__setitem__("updated", True)) + monkeypatch.setattr(mq, "post_comment", lambda *a, **k: None) + monkeypatch.setattr(mq, "add_label_by_name", lambda *a, **k: None) + return main_sha, head_sha + + +def test_process_once_auto_merges_unlabeled_ready_pr(monkeypatch): + """End-to-end: a fully-ready PR with NO merge-queue label is auto-merged. + This is the core autonomy fix — no human/agent labeling required.""" + calls = {"merged": None, "updated": False} + head_sha = "a" * 40 + _wire_ready_process_once( + monkeypatch, + issues=[_issue(90, labels=[])], # NO merge-queue label + pr_payload={ + "state": "open", "mergeable": True, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": head_sha, "repo_id": 1}, + "labels": [], + }, + calls=calls, + ) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + assert calls["merged"] == 90 # merged despite no merge-queue label + + +def test_process_once_skips_opt_out_labeled_pr(monkeypatch): + """A fully-ready PR carrying an opt-out label is NOT merged (skipped).""" + for optout in OPT_OUT: + calls = {"merged": None, "updated": False} + head_sha = "a" * 40 + _wire_ready_process_once( + monkeypatch, + issues=[_issue(91, labels=[optout])], + pr_payload={ + "state": "open", "mergeable": True, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": head_sha, "repo_id": 1}, + "labels": [{"name": optout}], + }, + calls=calls, + ) + rc = mq.process_once(dry_run=False) + assert rc == 0 + assert calls["merged"] is None, f"{optout!r} PR must not be merged" + + +def test_process_once_does_not_merge_unapproved_pr(monkeypatch): + """A not-ready PR (only one genuine approval) is auto-considered but NOT + merged — auto-discovery does not lower the merge bar.""" + calls = {"merged": None, "updated": False} + head_sha = "a" * 40 + main_sha, _ = _wire_ready_process_once( + monkeypatch, + issues=[_issue(92, labels=[])], + pr_payload={ + "state": "open", "mergeable": True, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": head_sha, "repo_id": 1}, + "labels": [], + }, + calls=calls, + ) + # Only ONE genuine approval → below the required 2. + monkeypatch.setattr(mq, "get_pull_reviews", lambda n: [ + {"state": "APPROVED", "user": {"login": "agent-researcher"}, + "official": True, "stale": False, "dismissed": False, "commit_id": head_sha}, + ]) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + assert calls["merged"] is None + + +def test_process_once_does_not_merge_red_required_pr(monkeypatch): + """A not-ready PR (required context red) is auto-considered but NOT merged.""" + calls = {"merged": None, "updated": False} + head_sha = "a" * 40 + main_sha = "b" * 40 + _wire_ready_process_once( + monkeypatch, + issues=[_issue(93, labels=[])], + pr_payload={ + "state": "open", "mergeable": True, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": head_sha, "repo_id": 1}, + "labels": [], + }, + calls=calls, + ) + + # Required PR context is FAILURE; main stays green. + def fake_combined(sha): + if sha == main_sha: + return {"state": "success", + "statuses": [{"context": "CI / all-required (push)", "status": "success"}]} + return {"state": "failure", + "statuses": [{"context": "CI / all-required (pull_request)", "status": "failure"}]} + monkeypatch.setattr(mq, "get_combined_status", fake_combined) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + assert calls["merged"] is None + + +def test_process_once_does_not_merge_unmergeable_pr(monkeypatch): + """A not-ready PR (mergeable False = conflicts) is auto-considered but NOT + merged.""" + calls = {"merged": None, "updated": False} + head_sha = "a" * 40 + _wire_ready_process_once( + monkeypatch, + issues=[_issue(94, labels=[])], + pr_payload={ + "state": "open", "mergeable": False, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": head_sha, "repo_id": 1}, + "labels": [], + }, + calls=calls, + ) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + assert calls["merged"] is None + + +def test_process_once_defensive_skip_when_pull_payload_opted_out(monkeypatch): + """If the listing missed an opt-out label but the authoritative pull payload + carries it (stale listing race), process_once must still skip the merge.""" + calls = {"merged": None, "updated": False} + head_sha = "a" * 40 + _wire_ready_process_once( + monkeypatch, + issues=[_issue(95, labels=[])], # listing shows no opt-out + pr_payload={ + "state": "open", "mergeable": True, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": head_sha, "repo_id": 1}, + "labels": [{"name": "do-not-auto-merge"}], # live pull is opted out + }, + calls=calls, + ) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + assert calls["merged"] is None diff --git a/.gitea/workflows/gitea-merge-queue.yml b/.gitea/workflows/gitea-merge-queue.yml index f5752a243..c8c4b23ea 100644 --- a/.gitea/workflows/gitea-merge-queue.yml +++ b/.gitea/workflows/gitea-merge-queue.yml @@ -7,10 +7,13 @@ name: gitea-merge-queue # the user-space queue bot, one PR per tick, using the non-bypass merge actor. # # Queue contract: -# - add label `merge-queue` to an open same-repo PR +# - auto-discovery (default): any open same-repo PR is considered — no +# `merge-queue` label required (the label is optional metadata now) # - bot updates stale PR heads with current main, then waits for CI -# - bot merges only when current main is green and required PR contexts pass -# - add `merge-queue-hold` to pause a queued PR without removing it +# - bot merges only when current main is green, genuine approvals are present +# on the current head, required PR contexts pass, and the PR is mergeable +# - add `merge-queue-hold`, `do-not-auto-merge`, or `wip` to keep a PR OUT of +# autonomous merging; draft PRs are also skipped on: # Schedule moved to operator-config: @@ -48,6 +51,19 @@ jobs: WATCH_BRANCH: ${{ github.event.repository.default_branch }} QUEUE_LABEL: merge-queue HOLD_LABEL: merge-queue-hold + # Auto-discovery (opt-OUT). When on (default), the queue considers ALL + # open same-repo PRs that meet the merge bar — it does NOT wait for a + # human/agent to add `merge-queue`. Agent Gitea tokens lack + # write:issue (labels are issue-scoped) and could never self-label, + # which stalled the queue; the label is now OPTIONAL metadata. The + # merge bar is UNCHANGED — only candidate selection widens. Set + # AUTO_DISCOVER=0 to restore legacy opt-IN (require the merge-queue + # label to be considered). + AUTO_DISCOVER: "1" + # Opt-OUT labels: any of these on a PR keeps it OUT of autonomous + # merging (the human escape hatch). HOLD_LABEL is always also honoured. + # A human who wants a PR held just adds one of these labels. + OPT_OUT_LABELS: do-not-auto-merge,wip UPDATE_STYLE: merge # Recognised official-reviewer set. A merge needs >= required_approvals # DISTINCT genuine official approvals from these accounts on the diff --git a/runbooks/gitea-merge-queue.md b/runbooks/gitea-merge-queue.md index 33893fbda..01976618d 100644 --- a/runbooks/gitea-merge-queue.md +++ b/runbooks/gitea-merge-queue.md @@ -8,26 +8,39 @@ against the latest `main`. ## Queue Contract -Add the `merge-queue` label to an open PR when it is ready to merge. +**Auto-discovery (opt-OUT, default).** You do NOT need to label a PR. The bot +auto-discovers every open same-repo PR and merges any that meets the bar. The +`merge-queue` label is now optional metadata, not a gate. This removed the +historical autonomy gap: agent Gitea tokens lack `write:issue` (labels are +issue-scoped), so agents could never self-label and ready PRs stalled. + +To keep a PR OUT of autonomous merging, add an opt-OUT label: +`merge-queue-hold`, `do-not-auto-merge`, or `wip`. Draft PRs are also skipped. The bot processes one PR per tick: -1. Confirms `main` is green. -2. Selects the oldest open PR carrying `merge-queue`. -3. Skips PRs with `merge-queue-hold`. -4. Rejects fork PRs because the queue may only update same-repo branches. -5. If the PR head does not contain current `main`, calls Gitea's +1. Confirms `main`'s branch-protection-required push contexts are green. +2. Selects the oldest open same-repo PR that is NOT opt-out-labeled and NOT a + draft (auto-discovery). With `AUTO_DISCOVER=0` it falls back to legacy + opt-IN: only PRs carrying `merge-queue` are considered. +3. Rejects fork PRs because the queue may only update same-repo branches. +4. If the PR head does not contain current `main`, calls Gitea's `/pulls/{n}/update?style=merge` endpoint and waits for CI on the new head. -6. Merges only after the current PR head has required contexts green: - - `CI / all-required (pull_request)` - - `sop-checklist / all-items-acked (pull_request)` +5. Merges only when, on the PR's CURRENT head sha: + - `>= required_approvals` distinct genuine official `APPROVED` reviews from + the recognised reviewer set (read from branch protection; default 2), + - no open official `REQUEST_CHANGES`, + - every branch-protection-required status context is green, and + - the PR is `mergeable` (Gitea returns `True`; `None`/`False` = wait). -The workflow is serialized with `concurrency`, so two queued PRs cannot be +The merge bar is unchanged by auto-discovery — only WHICH PRs are considered +changes. The workflow is serialized with `concurrency`, so two PRs cannot be merged against the same observed `main`. ## Operator Commands -Queue a PR: +Queue a PR (optional — auto-discovery already considers every ready PR; the +label is just visible metadata): ```bash curl -fsS -X POST \ @@ -37,7 +50,8 @@ curl -fsS -X POST \ -d '{"labels":["merge-queue"]}' ``` -Temporarily hold a queued PR: +Keep a PR OUT of autonomous merging (opt-OUT — use `merge-queue-hold`, +`do-not-auto-merge`, or `wip`): ```bash curl -fsS -X POST \ @@ -56,9 +70,11 @@ REPO=molecule-ai/molecule-core \ WATCH_BRANCH=main \ QUEUE_LABEL=merge-queue \ HOLD_LABEL=merge-queue-hold \ +AUTO_DISCOVER=1 \ +OPT_OUT_LABELS=do-not-auto-merge,wip \ +REVIEWER_SET=agent-reviewer,agent-researcher,agent-reviewer-cr2 \ UPDATE_STYLE=merge \ -REQUIRED_CONTEXTS='CI / all-required (pull_request),sop-checklist / all-items-acked (pull_request)' \ -python3 .gitea/scripts/gitea-merge-queue.py +python3 .gitea/scripts/gitea-merge-queue.py --dry-run ``` Dry run: -- 2.52.0 From 51f83260df21e0dbba425e40a9f890ba57e0bf29 Mon Sep 17 00:00:00 2001 From: devops-engineer Date: Sat, 6 Jun 2026 02:06:31 -0700 Subject: [PATCH 2/2] merge-queue: scan past non-ready candidates (HOL fix) + draft opt-out label MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Researcher REQUEST_CHANGES (review 9085, head 0c311bbc) caught a real head-of-line defect in the new auto-discovery: choose_next_candidate_issue() selected only the OLDEST non-opted-out PR and process_once() evaluated just that one per tick. A false candidate (e.g. #1519: open + unlabeled but mergeable=false, current-head official REQUEST_CHANGES, <2 genuine approvals) returns decision=wait and is re-selected every tick, HOL-blocking all newer ready PRs forever. Fix: - Add choose_candidate_issues() returning the FULL FIFO-ordered eligible list; process_once() now SCANS THROUGH it, skipping any `wait` candidate (REQUEST_CHANGES / mergeable!=True / insufficient genuine approvals / red required CI) and acting on the first ACTIONABLE one (an `update` that advances a stale branch, or a fully-ready `merge`). A non-ready PR no longer blocks newer ready PRs. The merge bar is UNCHANGED and fail-closed: a skipped PR is never merged. Per-PR evaluation factored into _evaluate_candidate(); the permanent-permission HOLD path now `continue`s the scan instead of returning. - Add literal `draft` to the default OPT_OUT_LABELS (Gitea draft STATE was already skipped; the label is an additional explicit human opt-out). Tests (§SOP-22): non-ready oldest is skipped and a newer ready PR merges in the same tick (no HOL); #1519-style false candidate is never merged and never blocks; red-required-CI candidate skipped for the ready PR; all-unready merges nothing; draft-label opt-out; choose_candidate_issues full-list ordering. 41 existing tests stay green (47 total). Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitea/scripts/gitea-merge-queue.py | 299 ++++++++++++------ .../scripts/tests/test_gitea_merge_queue.py | 277 +++++++++++++++- 2 files changed, 468 insertions(+), 108 deletions(-) diff --git a/.gitea/scripts/gitea-merge-queue.py b/.gitea/scripts/gitea-merge-queue.py index 18a41d68a..29b561a60 100644 --- a/.gitea/scripts/gitea-merge-queue.py +++ b/.gitea/scripts/gitea-merge-queue.py @@ -4,8 +4,11 @@ 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 same-repo PR that is NOT opted out (auto-discovery, - see below), skipping drafts. +1. Scan open same-repo PRs that are NOT opted out (auto-discovery, see below), + oldest-first, skipping drafts, until an ACTIONABLE one is found. A non-ready + candidate (REQUEST_CHANGES, mergeable!=True, insufficient genuine approvals, + or red required CI) is SKIPPED so it cannot head-of-line block newer ready + PRs; the scan continues to the next candidate. 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 @@ -44,19 +47,28 @@ Auto-discovery (opt-OUT, label-optional): is now OPTIONAL metadata, not a gate. SAFETY is preserved as opt-OUT: any PR carrying an opt-out label - (OPT_OUT_LABELS — `merge-queue-hold`, `do-not-auto-merge`, `wip` by default) is - skipped (never auto-considered, never merged). Draft PRs (draft=true) are also - skipped. A human who wants to keep a PR out of autonomous merging just adds one - of those labels. Setting AUTO_DISCOVER=0 restores the legacy opt-IN behaviour + (OPT_OUT_LABELS — `merge-queue-hold`, `do-not-auto-merge`, `wip`, `draft` by + default) is skipped (never auto-considered, never merged). Draft PRs + (draft=true STATE) are also skipped; the literal `draft` LABEL is an + additional explicit opt-out a human can apply without converting to a draft. + A human who wants to keep a PR out of autonomous merging just adds one of + those labels. Setting AUTO_DISCOVER=0 restores the legacy opt-IN behaviour (only PRs already carrying QUEUE_LABEL are considered). -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. Likewise, a -persistent branch-update conflict (the /update endpoint returns HTTP 409 -because the PR branch cannot be merged with main without manual rebase) HOLDS -the PR — a conflict will not self-resolve, so retrying it every tick would -HOL-block every ready PR behind it (issue #2352). +Head-of-line (HOL) safety has two complementary layers: + (a) The queue SCANS THROUGH the FIFO candidate list and skips any non-ready + PR (REQUEST_CHANGES, mergeable!=True, insufficient genuine approvals, or + red required CI) instead of locking on the oldest and waiting, so a PR + that can never become ready without human action does not block newer + ready PRs. + (b) For the candidate the scan acts on, two permanent failure modes HOLD the + PR (apply HOLD_LABEL) and let the scan CONTINUE to the next candidate + rather than re-selecting the same wedged PR every tick: + - a permanent permission/4xx merge error (403/404/405), and + - a persistent branch-update conflict (the /update endpoint returns + HTTP 409 because the PR branch cannot be merged with main without a + manual rebase). A conflict will not self-resolve, so retrying it + every tick would HOL-block every ready PR behind it (issue #2352). 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). @@ -105,11 +117,14 @@ AUTO_DISCOVER = _env("AUTO_DISCOVER", default="1").strip().lower() not in { # never merged) — the human escape hatch from autonomous merging. HOLD_LABEL is # always included so the existing hold semantics keep working. `do-not-auto-merge` # and `wip` let a human keep a PR out of the auto-merge path without removing it. +# `draft` is included as a literal label too: Gitea draft STATE (draft=true) is +# already skipped via _issue_is_draft, but a "draft" LABEL is an additional, +# explicit opt-out signal a human can apply without converting the PR to a draft. OPT_OUT_LABELS = { name.strip() for name in _env( "OPT_OUT_LABELS", - default="do-not-auto-merge,wip", + default="do-not-auto-merge,wip,draft", ).split(",") if name.strip() } | ({HOLD_LABEL} if HOLD_LABEL else set()) @@ -468,14 +483,14 @@ def _issue_is_draft(issue: dict) -> bool: return issue.get("draft") is True -def choose_next_candidate_issue( +def choose_candidate_issues( issues: list[dict], *, queue_label: str, opt_out_labels: set[str], auto_discover: bool, -) -> dict | None: - """Pick the oldest open PR eligible for a merge attempt this tick. +) -> list[dict]: + """All open PRs eligible for a merge attempt this tick, oldest-first. This is the auto-discovery selector. It does NOT change the merge bar — it only changes WHICH PRs are considered: @@ -488,8 +503,15 @@ def choose_next_candidate_issue( candidates (still skipping opt-out labels and drafts). Opt-out is the safety escape hatch: any opt_out_labels member present skips - the PR entirely (never considered, never merged). Selection is oldest-first + the PR entirely (never considered, never merged). Ordering is oldest-first (created_at, then number) to preserve the serialized FIFO ordering. + + Returns the FULL ordered list (not just the head) so process_once can SCAN + THROUGH non-ready candidates instead of locking on the oldest. A non-ready + auto-discovered PR (e.g. one with REQUEST_CHANGES or mergeable=false, which + can never become ready without human action) must NOT head-of-line block the + newer ready PRs behind it — the readiness check happens per-candidate in + process_once, and a `wait` candidate is skipped to the next one. """ candidates = [] for issue in issues: @@ -504,6 +526,26 @@ def choose_next_candidate_issue( continue # legacy opt-IN: require the queue label candidates.append(issue) candidates.sort(key=lambda issue: (issue.get("created_at") or "", int(issue["number"]))) + return candidates + + +def choose_next_candidate_issue( + issues: list[dict], + *, + queue_label: str, + opt_out_labels: set[str], + auto_discover: bool, +) -> dict | None: + """The oldest eligible candidate, or None. Thin head-of-list wrapper around + choose_candidate_issues; retained for callers/tests that only want the head. + process_once uses the full list (choose_candidate_issues) so it can scan past + non-ready PRs rather than HOL-block on the oldest.""" + candidates = choose_candidate_issues( + issues, + queue_label=queue_label, + opt_out_labels=opt_out_labels, + auto_discover=auto_discover, + ) return candidates[0] if candidates else None @@ -853,59 +895,181 @@ def process_once(*, dry_run: bool = False) -> int: print(f"::notice::queue paused: {WATCH_BRANCH}@{main_sha[:8]} required contexts not green: {', '.join(main_bad)}") return 0 - issue = choose_next_candidate_issue( + candidates = choose_candidate_issues( list_candidate_issues(auto_discover=AUTO_DISCOVER), queue_label=QUEUE_LABEL, opt_out_labels=OPT_OUT_LABELS, auto_discover=AUTO_DISCOVER, ) - if not issue: + if not candidates: print( "::notice::no merge candidates " f"(auto_discover={'on' if AUTO_DISCOVER else 'off'})" ) return 0 + # HOL fix: SCAN THROUGH the FIFO candidate list until a PR we can ACT on is + # found, instead of locking on the oldest and waiting. A non-ready candidate + # (decision.action == "wait": REQUEST_CHANGES, mergeable!=True, insufficient + # genuine approvals, or red required CI) is SKIPPED — it must NOT head-of-line + # block the newer ready PRs behind it. The merge bar is unchanged: a skipped + # PR is never merged, and the first ACTIONABLE candidate (an "update" that + # advances a stale branch, or a fully-ready "merge") terminates the scan. + # + # `update` is treated as actionable, not skippable: a PR whose head merely + # lacks current main is in a legitimate in-progress state (updating it + + # rerunning CI moves it toward ready), unlike a PR that can never become + # ready without a human (RC / conflict), which is a `wait` and gets skipped. + for issue in candidates: + decision, ctx = _evaluate_candidate( + issue, + main_sha=main_sha, + main_status=main_status, + required_contexts=contexts, + required_approvals=required_approvals, + dry_run=dry_run, + ) + if decision is None: + continue # not merge-eligible (not-open / opted-out / fork / wrong base) + pr_number = ctx["pr_number"] + print(f"::notice::PR #{pr_number} decision={decision.action}: {decision.reason}") + if decision.action == "wait": + # Non-ready: skip to the next candidate (no HOL block, no merge). + continue + if decision.action == "update": + try: + update_pull(pr_number, dry_run=dry_run) + except BranchUpdateConflictError as exc: + # The branch cannot be updated with main because of a real + # conflict (HTTP 409 from /update). This is the #2352 HOL guard: + # a conflict will not self-resolve without a human/agent rebase, + # so re-attempting the update every tick would head-of-line block + # every ready PR behind it. HOLD this PR (apply HOLD_LABEL, which + # is an opt-out label so later ticks skip it) and CONTINUE the + # scan so a newer ready PR can still merge this tick. Fail-closed: + # a held PR is skipped, never merged. + sys.stderr.write( + f"::error::branch-update conflict for PR #{pr_number}: {exc}\n" + ) + hold_note = ( + "merge-queue: could not update this branch with " + f"`{WATCH_BRANCH}` — the update returned a merge conflict " + f"(HTTP 409) that the queue cannot auto-resolve ({exc}). " + f"Applied `{HOLD_LABEL}` to unblock the queue (HOL guard). " + f"Fix: rebase/merge `{WATCH_BRANCH}` into this branch and " + f"resolve the conflicts, then remove `{HOLD_LABEL}` to requeue." + ) + hold_pr(pr_number, hold_note, dry_run=dry_run) + continue # held — keep scanning for a mergeable candidate + post_comment( + pr_number, + ( + f"merge-queue: updated this branch with `{WATCH_BRANCH}` at " + f"`{main_sha[:12]}`. Waiting for CI on the refreshed head." + ), + dry_run=dry_run, + ) + return 0 + if decision.ready: + latest_main_sha = get_branch_head(WATCH_BRANCH) + if latest_main_sha != main_sha: + print( + f"::notice::main moved {main_sha[:8]} -> {latest_main_sha[:8]}; " + "deferring to next tick" + ) + return 0 + try: + merge_pull(pr_number, dry_run=dry_run, force=decision.force) + except MergePermissionError as exc: + # Permanent merge failure (HTTP 403/404/405). HOLD this PR by + # applying HOLD_LABEL (it becomes an opt-out label, so subsequent + # ticks skip it) and CONTINUE scanning so the queue still advances + # to the next ready PR this tick rather than stalling. + sys.stderr.write(f"::error::merge permission error for PR #{pr_number}: {exc}\n") + 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) + continue # held — keep scanning for a mergeable candidate + return 0 + return 0 + + +def _evaluate_candidate( + issue: dict, + *, + main_sha: str, + main_status: dict, + required_contexts: list[str], + required_approvals: int, + dry_run: bool, +) -> tuple[MergeDecision | None, dict]: + """Evaluate a single auto-discovered candidate against the full merge bar. + + Returns (decision, ctx) where ctx carries {"pr_number"}. A None decision + means the PR is not merge-eligible at all (not open / opted-out / draft / + fork / wrong base) and the caller should skip to the next candidate; for + fork / wrong-base the explanatory comment is posted here before returning. + + The merge bar is UNCHANGED from the single-PR path — this only factors the + per-PR evaluation out so process_once can scan multiple candidates. A failed + status fetch still raises (fail-closed): it propagates to the caller so the + PR is never treated as green. + """ pr_number = int(issue["number"]) + ctx = {"pr_number": pr_number} pr = get_pull(pr_number) if pr.get("state") != "open": print(f"::notice::PR #{pr_number} is not open; skipping") - return 0 + return None, ctx # Defensive opt-out/draft re-check on the authoritative pull payload: the # /issues listing's label/draft view can lag, but the merge bar must respect - # the live pull state. (choose_next_candidate_issue already filtered on the + # the live pull state. (choose_candidate_issues already filtered on the # listing; this guards against a stale listing racing a just-added opt-out.) if OPT_OUT_LABELS & label_names(pr): print(f"::notice::PR #{pr_number} carries an opt-out label; skipping") - return 0 + return None, ctx if pr.get("draft") is True: print(f"::notice::PR #{pr_number} is a draft; skipping") - return 0 + return None, ctx if pr.get("base", {}).get("ref") != WATCH_BRANCH: post_comment(pr_number, f"merge-queue: skipped; base branch is not `{WATCH_BRANCH}`.", dry_run=dry_run) - return 0 + return None, ctx if pr.get("head", {}).get("repo_id") != pr.get("base", {}).get("repo_id"): post_comment(pr_number, "merge-queue: skipped; fork PRs are not supported by the serialized queue.", dry_run=dry_run) - return 0 + return None, ctx head_sha = pr.get("head", {}).get("sha") if not isinstance(head_sha, str) or len(head_sha) < 7: 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). + # Fail-closed: a failed status fetch raises here and propagates (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 + # mergeable so evaluate_merge_readiness returns a "wait" decision. + mergeable = pr.get("mergeable") is True reviews = get_pull_reviews(pr_number) approvers, request_changes = genuine_approvals( @@ -915,7 +1079,7 @@ def process_once(*, dry_run: bool = False) -> int: decision = evaluate_merge_readiness( main_status=main_status, pr_status=pr_status, - required_contexts=contexts, + required_contexts=required_contexts, required_approvals=required_approvals, approvers=approvers, request_changes=request_changes, @@ -923,72 +1087,7 @@ def process_once(*, dry_run: bool = False) -> int: mergeable=mergeable, pr_labels=pr_labels, ) - - print(f"::notice::PR #{pr_number} decision={decision.action}: {decision.reason}") - if decision.action == "update": - try: - update_pull(pr_number, dry_run=dry_run) - except BranchUpdateConflictError as exc: - # The branch cannot be updated with main because of a real conflict - # (HTTP 409). This is the HOL fix for issue #2352: previously the - # 409 propagated to main() and the tick exited 0 with the PR still - # queued, so the NEXT tick re-selected the SAME conflicted PR and - # retried the failing update forever — head-of-line-blocking every - # ready PR behind it. A conflict will not self-resolve; it needs a - # human/agent rebase. So HOLD this PR (HOL guard) and advance to the - # next candidate. Fail-closed: a held PR is skipped, never merged. - sys.stderr.write( - f"::error::branch-update conflict for PR #{pr_number}: {exc}\n" - ) - hold_note = ( - "merge-queue: could not update this branch with " - f"`{WATCH_BRANCH}` — the update returned a merge conflict " - f"(HTTP 409) that the queue cannot auto-resolve ({exc}). " - f"Applied `{HOLD_LABEL}` to unblock the queue (HOL guard). " - f"Fix: rebase/merge `{WATCH_BRANCH}` into this branch and " - f"resolve the conflicts, then remove `{HOLD_LABEL}` to requeue." - ) - hold_pr(pr_number, hold_note, dry_run=dry_run) - return 0 - post_comment( - pr_number, - ( - f"merge-queue: updated this branch with `{WATCH_BRANCH}` at " - f"`{main_sha[:12]}`. Waiting for CI on the refreshed head." - ), - dry_run=dry_run, - ) - return 0 - if decision.ready: - latest_main_sha = get_branch_head(WATCH_BRANCH) - if latest_main_sha != main_sha: - print( - f"::notice::main moved {main_sha[:8]} -> {latest_main_sha[:8]}; " - "deferring to next tick" - ) - return 0 - try: - merge_pull(pr_number, dry_run=dry_run, force=decision.force) - except MergePermissionError as exc: - # 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") - 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." - ) - hold_pr(pr_number, hold_note, dry_run=dry_run) - return 0 - return 0 - return 0 + return decision, ctx def main() -> int: diff --git a/.gitea/scripts/tests/test_gitea_merge_queue.py b/.gitea/scripts/tests/test_gitea_merge_queue.py index c2366ec55..83740b6a5 100644 --- a/.gitea/scripts/tests/test_gitea_merge_queue.py +++ b/.gitea/scripts/tests/test_gitea_merge_queue.py @@ -601,6 +601,8 @@ def _stale_pr_update_409_monkeypatch(monkeypatch, queued_issues, calls): monkeypatch.setattr(mq, "WATCH_BRANCH", "main") monkeypatch.setattr(mq, "QUEUE_LABEL", "merge-queue") monkeypatch.setattr(mq, "HOLD_LABEL", "merge-queue-hold") + monkeypatch.setattr(mq, "AUTO_DISCOVER", True) + monkeypatch.setattr(mq, "OPT_OUT_LABELS", {"merge-queue-hold", "do-not-auto-merge", "wip"}) monkeypatch.setattr(mq, "REVIEWER_SET", REVIEWERS) monkeypatch.setattr(mq, "get_branch_protection", lambda branch: mq.BranchProtection( required_contexts=["CI / all-required (pull_request)"], @@ -616,7 +618,8 @@ def _stale_pr_update_409_monkeypatch(monkeypatch, queued_issues, calls): return {"state": "success", "statuses": [{"context": ctx, "status": "success"}]} monkeypatch.setattr(mq, "get_combined_status", fake_combined) - monkeypatch.setattr(mq, "list_queued_issues", lambda: queued_issues) + # Scan-loop process_once enumerates candidates via list_candidate_issues. + monkeypatch.setattr(mq, "list_candidate_issues", lambda *, auto_discover: queued_issues) monkeypatch.setattr(mq, "get_pull", lambda n: { "state": "open", "number": n, "mergeable": True, "base": {"ref": "main", "repo_id": 1}, @@ -646,6 +649,7 @@ def _stale_pr_update_409_monkeypatch(monkeypatch, queued_issues, calls): def fake_add_label(pr_number, label_name, *, dry_run): calls["hold_label"] = (pr_number, label_name) + calls.setdefault("holds", []).append((pr_number, label_name)) monkeypatch.setattr(mq, "add_label_by_name", fake_add_label) monkeypatch.setattr(mq, "post_comment", lambda *a, **k: None) @@ -675,9 +679,12 @@ def test_process_once_holds_pr_on_409_conflict_on_update(monkeypatch): def test_queue_advances_past_held_conflicted_pr(monkeypatch): - """End-to-end HOL proof for #2352: PR #1409 (oldest) hits a 409-on-update - and is held; on the NEXT tick choose_next_queued_issue must SKIP the held - PR and select the next ready PR (#1500) instead of stalling on #1409.""" + """End-to-end HOL proof for #2352 under the scan-loop architecture: PR #1409 + (oldest) hits a 409-on-update and is HELD (HOLD_LABEL applied); once held it + carries an opt-out label so it is excluded from candidate selection and can + never re-block the queue. The 409-conflict hold (#2354) and the + scan-through-skip (#2356) coexist: a held conflicted PR is both held AND no + longer a candidate, so newer ready PRs behind it are unblocked.""" calls = {"update_attempts": 0, "merge_attempts": 0, "hold_label": None} conflicted = {"number": 1409, "pull_request": {}, "labels": [{"name": "merge-queue"}], @@ -691,16 +698,30 @@ def test_queue_advances_past_held_conflicted_pr(monkeypatch): calls=calls, ) - # Tick 1: oldest (#1409) is selected, 409-on-update → held. + # Tick 1: oldest (#1409) is selected, 409-on-update → held, then the scan + # CONTINUES to #1500 (which also 409s in this fixture and is likewise held). + # The key #2352 property: the conflicted oldest PR is held and does NOT stop + # the scan from advancing past it. rc = mq.process_once(dry_run=False) assert rc == 0 - assert calls["hold_label"] == (1409, "merge-queue-hold") + assert (1409, "merge-queue-hold") in calls["holds"] + assert calls["merge_attempts"] == 0 # held, not merged — fail-closed # Simulate the label now present on #1409 (as the real hold would persist). conflicted["labels"] = [{"name": "merge-queue"}, {"name": "merge-queue-hold"}] - # Tick 2: the queue must ADVANCE — choose_next_queued_issue skips the held - # #1409 and selects the next ready candidate #1500, NOT re-select #1409. + # Next selection: the scan-loop candidate selector must SKIP the now-held + # #1409 (HOLD_LABEL is in OPT_OUT_LABELS) and surface the next ready + # candidate #1500 — the held PR no longer head-of-line blocks. The legacy + # opt-IN selector (choose_next_queued_issue) honours the same hold. + opt_out = {"merge-queue-hold", "do-not-auto-merge", "wip"} + remaining = mq.choose_candidate_issues( + [conflicted, next_ready], + queue_label="merge-queue", + opt_out_labels=opt_out, + auto_discover=True, + ) + assert [i["number"] for i in remaining] == [1500] selected = mq.choose_next_queued_issue( [conflicted, next_ready], queue_label="merge-queue", @@ -1007,6 +1028,246 @@ def test_process_once_does_not_merge_unmergeable_pr(monkeypatch): assert calls["merged"] is None +# -------------------------------------------------------------------------- +# §SOP-22 (cont.): HEAD-OF-LINE (HOL) — a non-ready auto-discovered candidate +# must NOT block the newer ready PRs behind it. The queue SCANS THROUGH the +# FIFO candidate list, skipping `wait` candidates (REQUEST_CHANGES, mergeable +# != True, insufficient genuine approvals, or red required CI), and merges the +# first ready PR in the SAME tick. (Regression for the #1519-style false +# candidate the reviewer caught: open + unlabeled + mergeable=false + current- +# head official REQUEST_CHANGES + <2 genuine approvals.) +# -------------------------------------------------------------------------- + +MAIN_SHA = "b" * 40 + + +def _wire_multi_candidate_process_once(monkeypatch, *, issues, pulls, reviews, calls): + """Wire process_once for MULTIPLE candidates, dispatching get_pull / + get_pull_reviews / head-status BY PR NUMBER so each candidate can have a + different readiness. `pulls` maps number -> pull payload; `reviews` maps + number -> reviews list. Main is green; each PR head status is green.""" + 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, "AUTO_DISCOVER", True) + monkeypatch.setattr(mq, "OPT_OUT_LABELS", OPT_OUT) + 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, + )) + 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_candidate_issues", lambda *, auto_discover: issues) + monkeypatch.setattr(mq, "get_pull", lambda n: dict(pulls[n], number=n)) + # Each PR head contains current main (so no candidate needs an update; the + # only differentiator is readiness). head sha is the pull's own head. + monkeypatch.setattr( + mq, "get_pull_commits", + lambda n: [{"sha": MAIN_SHA}, {"sha": pulls[n]["head"]["sha"]}], + ) + monkeypatch.setattr(mq, "get_pull_reviews", lambda n: reviews[n]) + + def fake_merge(pr_number, *, dry_run, force=False): + calls.setdefault("merged", []) + calls["merged"].append(pr_number) + monkeypatch.setattr(mq, "merge_pull", fake_merge) + monkeypatch.setattr(mq, "update_pull", lambda *a, **k: calls.__setitem__("updated", True)) + monkeypatch.setattr(mq, "post_comment", lambda *a, **k: None) + monkeypatch.setattr(mq, "add_label_by_name", lambda *a, **k: None) + + +def _two_approvals(head_sha): + return [ + {"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 test_hol_unready_oldest_does_not_block_newer_ready_pr(monkeypatch): + """The OLDEST auto-discovered candidate is NOT ready (mergeable=false). The + queue must SKIP it and merge the NEWER ready PR in the SAME tick — no HOL.""" + calls = {"updated": False} + old_head, new_head = "a" * 40, "c" * 40 + _wire_multi_candidate_process_once( + monkeypatch, + issues=[ + _issue(500, labels=[], created="2026-06-01T01:00:00Z"), # oldest, NOT ready + _issue(501, labels=[], created="2026-06-01T02:00:00Z"), # newer, READY + ], + pulls={ + 500: {"state": "open", "mergeable": False, "draft": False, # conflict + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": old_head, "repo_id": 1}, "labels": []}, + 501: {"state": "open", "mergeable": True, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": new_head, "repo_id": 1}, "labels": []}, + }, + reviews={500: _two_approvals(old_head), 501: _two_approvals(new_head)}, + calls=calls, + ) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + # The newer ready PR merged; the non-ready oldest did not block it. + assert calls.get("merged") == [501] + + +def test_hol_1519_style_false_candidate_never_merged_and_never_blocks(monkeypatch): + """Live #1519 repro: oldest, open, UNLABELED, but mergeable=false + a + current-head official REQUEST_CHANGES + only ONE genuine approval. It must + NEVER be merged and must NEVER block the newer ready PR behind it.""" + calls = {"updated": False} + false_head, ready_head = "a" * 40, "c" * 40 + _wire_multi_candidate_process_once( + monkeypatch, + issues=[ + _issue(1519, labels=[], created="2026-05-20T00:00:00Z"), # oldest false candidate + _issue(2000, labels=[], created="2026-06-01T00:00:00Z"), # newer, READY + ], + pulls={ + 1519: {"state": "open", "mergeable": False, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": false_head, "repo_id": 1}, "labels": []}, + 2000: {"state": "open", "mergeable": True, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": ready_head, "repo_id": 1}, "labels": []}, + }, + reviews={ + 1519: [ + # one genuine approval (below 2) ... + {"state": "APPROVED", "user": {"login": "agent-researcher"}, + "official": True, "stale": False, "dismissed": False, "commit_id": false_head}, + # ... plus a current-head official REQUEST_CHANGES (human action needed) + {"state": "REQUEST_CHANGES", "user": {"login": "agent-reviewer"}, + "official": True, "stale": False, "dismissed": False, "commit_id": false_head}, + ], + 2000: _two_approvals(ready_head), + }, + calls=calls, + ) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + # #1519 is never merged; the ready PR behind it merges this same tick. + assert calls.get("merged") == [2000] + assert 1519 not in calls.get("merged", []) + + +def test_hol_unready_red_required_ci_is_skipped_for_ready_pr(monkeypatch): + """A candidate whose required CI is RED is skipped (not waited-on) so the + newer ready PR merges in the same tick.""" + calls = {"updated": False} + red_head, ready_head = "a" * 40, "c" * 40 + _wire_multi_candidate_process_once( + monkeypatch, + issues=[ + _issue(600, labels=[], created="2026-06-01T01:00:00Z"), # required CI red + _issue(601, labels=[], created="2026-06-01T02:00:00Z"), # ready + ], + pulls={ + 600: {"state": "open", "mergeable": True, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": red_head, "repo_id": 1}, "labels": []}, + 601: {"state": "open", "mergeable": True, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": ready_head, "repo_id": 1}, "labels": []}, + }, + reviews={600: _two_approvals(red_head), 601: _two_approvals(ready_head)}, + calls=calls, + ) + # PR 600's required PR context is FAILURE; 601 (and main) stay green. + def fake_combined(sha): + if sha == MAIN_SHA: + return {"state": "success", + "statuses": [{"context": "CI / all-required (push)", "status": "success"}]} + state = "failure" if sha == red_head else "success" + return {"state": state, + "statuses": [{"context": "CI / all-required (pull_request)", "status": state}]} + monkeypatch.setattr(mq, "get_combined_status", fake_combined) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + assert calls.get("merged") == [601] + + +def test_hol_all_candidates_unready_merges_nothing(monkeypatch): + """If EVERY candidate is non-ready, the queue merges nothing (fail-closed) + and does not loop — it simply finds no actionable PR this tick.""" + calls = {"updated": False} + h1, h2 = "a" * 40, "c" * 40 + _wire_multi_candidate_process_once( + monkeypatch, + issues=[ + _issue(700, labels=[], created="2026-06-01T01:00:00Z"), # RC + _issue(701, labels=[], created="2026-06-01T02:00:00Z"), # unmergeable + ], + pulls={ + 700: {"state": "open", "mergeable": True, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": h1, "repo_id": 1}, "labels": []}, + 701: {"state": "open", "mergeable": False, "draft": False, + "base": {"ref": "main", "repo_id": 1}, + "head": {"sha": h2, "repo_id": 1}, "labels": []}, + }, + reviews={ + 700: _two_approvals(h1) + [ + {"state": "REQUEST_CHANGES", "user": {"login": "agent-reviewer"}, + "official": True, "stale": False, "dismissed": False, "commit_id": h1}, + ], + 701: _two_approvals(h2), + }, + calls=calls, + ) + + rc = mq.process_once(dry_run=False) + + assert rc == 0 + assert calls.get("merged") is None # nothing merged; no HOL loop + + +def test_opt_out_draft_label_excludes_candidate(): + """The literal `draft` label is now an opt-out label (added to the default + OPT_OUT_LABELS), independent of Gitea draft STATE — a human can opt a PR out + by labeling it `draft` without converting it to a draft PR.""" + # `draft` must be in the shipped default opt-out set. + assert "draft" in mq.OPT_OUT_LABELS + opt_out = OPT_OUT | {"draft"} + issues = [_issue(800, labels=["draft"], draft=False)] # label only, not draft STATE + selected = mq.choose_next_candidate_issue( + issues, queue_label="merge-queue", opt_out_labels=opt_out, auto_discover=True + ) + assert selected is None + + +def test_choose_candidate_issues_returns_full_fifo_list_skipping_opt_out(): + """choose_candidate_issues returns ALL eligible candidates oldest-first (so + process_once can scan past non-ready ones), skipping opt-out/draft/non-PR.""" + issues = [ + _issue(72, labels=["merge-queue"], created="2026-06-01T03:00:00Z"), + _issue(70, labels=["do-not-auto-merge"], created="2026-06-01T01:00:00Z"), # opt-out + _issue(71, labels=[], created="2026-06-01T02:00:00Z"), + _issue(73, labels=[], draft=True, created="2026-06-01T00:30:00Z"), # draft + _issue(74, labels=[], is_pr=False, created="2026-06-01T00:00:00Z"), # not a PR + ] + ordered = mq.choose_candidate_issues( + issues, queue_label="merge-queue", opt_out_labels=OPT_OUT, auto_discover=True + ) + assert [i["number"] for i in ordered] == [71, 72] # FIFO, opt-out/draft/non-PR dropped + + def test_process_once_defensive_skip_when_pull_payload_opted_out(monkeypatch): """If the listing missed an opt-out label but the authoritative pull payload carries it (stale listing race), process_once must still skip the merge.""" -- 2.52.0