From d4044613b065712414109788b13dd02ed8b67a5c Mon Sep 17 00:00:00 2001 From: hongming-ceo-delegated Date: Wed, 24 Jun 2026 01:48:03 -0700 Subject: [PATCH] =?UTF-8?q?fix(merge-queue):=20close=20two=20auto-merge=20?= =?UTF-8?q?fail-opens=20=E2=80=94=20zero-approval=20merge=20+=20out-of-ros?= =?UTF-8?q?ter=20REQUEST=5FCHANGES=20dropped=20(#3210)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FIX 1 (CRITICAL): parse_branch_protection coerced a bool required_approvals via isinstance(True,int) (True->1, halving a 2-bar) and passed 0/negative straight through; evaluate_merge_readiness then had len(approvers)<0 always False -> a PR could auto-merge with ZERO genuine approvals, defeating the 2-genuine gate. Fix: reject bool/non-int -> default floor; clamp valid ints UP via max(approvals, REQUIRED_APPROVALS_DEFAULT) (a stricter BP wins; 0/negative never lower the bar); defence-in-depth floor max(required,1) on the non-exempt path in evaluate_merge_readiness. Runtime-bump exemption (legitimately 0 — a bot cannot self-approve) preserved + tested. FIX 2 (HIGH): classify_reviews applied the reviewer_set roster filter to BOTH verdicts before the verdict was known, so an official current-head REQUEST_CHANGES from a NON-roster login (e.g. the CTO/founder) was silently dropped and did not block the merge. Fix: reduce over ALL users; apply the roster gate ONLY to the approver tally (is_genuine_approval reviewer_set=...); any official current-head REQUEST_CHANGES from ANY login now blocks. Approver strictness byte-for-byte unchanged; the review-check.sh path (is_genuine_approval direct, reviewer_set=None) is unaffected. Both are strictly tightening — the gate now blocks cases it previously let through. Tests: +13 across test_gitea_merge_queue.py + test_approval_validator.py covering 0/neg/bool/missing/stricter approvals, exemption-preserved, and out-of-roster RC blocks; suite 199 passed. New tests proven to fail against the pre-fix behavior. Addresses #3210 (the merge-gate fail-open family — CRITICAL zero-approval + HIGH out-of-roster-RC). Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitea/scripts/_approval_validator.py | 22 ++- .gitea/scripts/gitea-merge-queue.py | 41 +++++- .../scripts/tests/test_approval_validator.py | 66 +++++++++ .../scripts/tests/test_gitea_merge_queue.py | 139 ++++++++++++++++++ 4 files changed, 261 insertions(+), 7 deletions(-) diff --git a/.gitea/scripts/_approval_validator.py b/.gitea/scripts/_approval_validator.py index 8356c584..99ad607a 100644 --- a/.gitea/scripts/_approval_validator.py +++ b/.gitea/scripts/_approval_validator.py @@ -212,6 +212,20 @@ def classify_reviews( # cannot overwrite or erase a genuine review. A user's verdict is the # state of their latest VALID (official, current-head, non-dismissed, # non-stale, commit_id-present-and-matching) review. + # + # REVIEWER_SET DECOUPLING (internal#3210 FIX-2, fail-open). + # + # The `reviewer_set` membership filter must apply ONLY to the APPROVER + # tally — it is the "recognised reviewers count toward the N-genuine + # floor" roster. It must NOT gate the REQUEST_CHANGES (block) set: an + # official, current-head, non-stale, non-dismissed REQUEST_CHANGES from + # ANY login (e.g. the CTO/founder or any non-roster reviewer) MUST block + # the merge regardless of roster membership. The earlier code applied the + # `user not in reviewer_set_set: continue` filter in this reduce loop, + # BEFORE the verdict was known, so an out-of-roster REQUEST_CHANGES was + # silently dropped and did not block — a fail-open. We therefore reduce + # over ALL users here (no roster gate) and apply the roster check only at + # the `approvers.add(...)` step below. latest_valid_by_user: dict = {} for review in reviews: if not isinstance(review, dict): @@ -219,8 +233,6 @@ def classify_reviews( user = (review.get("user") or {}).get("login") if not isinstance(user, str): continue - if reviewer_set_set is not None and user not in reviewer_set_set: - continue # EXACT-ENUM (fail-closed): exact constants only, no coercion. A # case-coerced row must not become eligible to overwrite/erase a # genuine per-user verdict in the reduce below. @@ -240,7 +252,11 @@ def classify_reviews( # Each surviving review already passed is_official_current_head, so # the state alone determines the verdict. We still go through the # per-verdict SSOT predicates so the rule cannot drift. - if is_genuine_approval(review, headsha=headsha, reviewer_set=None): + # + # APPROVERS: gated on reviewer_set membership (recognised-reviewer + # roster). REQUEST_CHANGES: NEVER gated — any official current-head + # block counts, regardless of roster (internal#3210 FIX-2). + if is_genuine_approval(review, headsha=headsha, reviewer_set=reviewer_set_set): approvers.add(user) elif is_open_request_changes(review, headsha=headsha): request_changes.append(user) diff --git a/.gitea/scripts/gitea-merge-queue.py b/.gitea/scripts/gitea-merge-queue.py index 95da8949..3289c6d3 100644 --- a/.gitea/scripts/gitea-merge-queue.py +++ b/.gitea/scripts/gitea-merge-queue.py @@ -713,10 +713,29 @@ def parse_branch_protection(body: Any) -> BranchProtection: raise BranchProtectionUnavailable( "enable_status_check is true but status_check_contexts is empty" ) + # FAIL-CLOSED approval floor (internal#3210 FIX-1). + # + # `required_approvals` is the genuine-approval bar consumed by + # evaluate_merge_readiness step 3. A degraded BP value here would silently + # weaken or zero that gate, so we clamp it UP to the default floor and never + # accept a value that lowers/skips it: + # + # - bool: `isinstance(True, int)` is True in Python, so an upstream bool + # True would coerce to int 1 and HALVE a 2-genuine bar to 1. Reject bool + # explicitly → fall back to the default floor. + # - 0 / negative: an admin lowering the bar, a Gitea migration/default + # reset, or a blanked/restored BP record can yield 0 (or a negative that + # passes a naive `< N` check). These must clamp UP to the default, NEVER + # down/skip. + # - valid positive int >= default: honored as-is (a stricter BP wins). + # + # max(int(approvals), REQUIRED_APPROVALS_DEFAULT) gives exactly this: the + # derived bar is never below the SSOT default floor. approvals = body.get("required_approvals") - required_approvals = ( - int(approvals) if isinstance(approvals, int) else REQUIRED_APPROVALS_DEFAULT - ) + if isinstance(approvals, bool) or not isinstance(approvals, int): + required_approvals = REQUIRED_APPROVALS_DEFAULT + else: + required_approvals = max(approvals, REQUIRED_APPROVALS_DEFAULT) return BranchProtection( required_contexts=contexts, required_approvals=required_approvals, @@ -1420,7 +1439,21 @@ def evaluate_merge_readiness( # bar, which a bot cannot satisfy by construction. Required # approvals is forced to 0 for the check below when the exemption # holds. - effective_required_approvals = 0 if runtime_bump_exempt else required_approvals + # + # DEFENSIVE FLOOR (internal#3210 FIX-1, defence-in-depth). On the + # NON-exempt path the bar can never legitimately be below 1 genuine + # approval — a single upstream `required_approvals == 0` (degraded BP, + # a caller that bypassed parse_branch_protection, a future refactor) + # must NOT be able to zero the genuine-approval gate. parse_branch_ + # protection already clamps the derived value up to the default floor; + # this is the second, independent guard at the consumption site so the + # two would both have to fail to open the gate. The runtime-bump + # exemption path is UNTOUCHED — it legitimately zeroes the HUMAN bar + # (a bot cannot self-approve), and the floor must not break it. + if runtime_bump_exempt: + effective_required_approvals = 0 + else: + effective_required_approvals = max(required_approvals, 1) if len(approvers) < effective_required_approvals: return MergeDecision( False, "wait", diff --git a/.gitea/scripts/tests/test_approval_validator.py b/.gitea/scripts/tests/test_approval_validator.py index c7a8ef68..a27bcc46 100644 --- a/.gitea/scripts/tests/test_approval_validator.py +++ b/.gitea/scripts/tests/test_approval_validator.py @@ -327,6 +327,72 @@ class ClassifyReviewsContract(unittest.TestCase): ) self.assertEqual(approvers, {"alice"}) + # ----------------------------------------------------------------- + # FIX-2 (internal#3210, HIGH fail-open): reviewer_set DECOUPLING. + # + # The reviewer_set roster gates ONLY the approver tally. An official, + # current-head, non-stale, non-dismissed REQUEST_CHANGES from a login + # OUTSIDE the roster (e.g. the CTO/founder, or any non-roster reviewer) + # MUST still block the merge. The earlier code applied the roster + # membership filter in the reduce loop BEFORE the verdict was known, so + # an out-of-roster REQUEST_CHANGES was silently dropped and did not + # block — exactly the fail-open these tests pin shut. + # ----------------------------------------------------------------- + + def test_out_of_roster_request_changes_still_blocks(self): + """An out-of-roster official current-head REQUEST_CHANGES MUST block, + while in-roster approvals are still tallied normally.""" + reviews = [ + _review(user="alice", state="APPROVED", commit_id=HEAD), + _review(user="bob", state="APPROVED", commit_id=HEAD), + # CTO/founder is NOT in the reviewer roster, but their + # REQUEST_CHANGES must not evaporate. + _review(user="cto-founder", state="REQUEST_CHANGES", commit_id=HEAD), + ] + approvers, request_changes = classify_reviews( + reviews, headsha=HEAD, reviewer_set={"alice", "bob"} + ) + # Roster approvals are counted (out-of-roster gate unchanged for them). + self.assertEqual(approvers, {"alice", "bob"}) + # The out-of-roster block is honored. + self.assertIn("cto-founder", request_changes) + + def test_out_of_roster_approval_still_excluded(self): + """The roster gate is unchanged for APPROVERS: an out-of-roster + APPROVED still does NOT count toward the genuine-approval floor.""" + reviews = [ + _review(user="alice", state="APPROVED", commit_id=HEAD), + _review(user="outsider", state="APPROVED", commit_id=HEAD), + ] + approvers, request_changes = classify_reviews( + reviews, headsha=HEAD, reviewer_set={"alice"} + ) + self.assertEqual(approvers, {"alice"}) + self.assertEqual(request_changes, []) + + def test_out_of_roster_request_changes_blocks_with_no_roster_approvals(self): + """Even with zero in-roster approvers, an out-of-roster + REQUEST_CHANGES is surfaced (the merge-queue step-2 gate then waits).""" + reviews = [ + _review(user="random-non-roster", state="REQUEST_CHANGES", commit_id=HEAD), + ] + approvers, request_changes = classify_reviews( + reviews, headsha=HEAD, reviewer_set={"alice", "bob"} + ) + self.assertEqual(approvers, set()) + self.assertEqual(request_changes, ["random-non-roster"]) + + def test_out_of_roster_stale_request_changes_does_not_block(self): + """The decouple does NOT weaken the fail-closed head/stale contract: + an out-of-roster REQUEST_CHANGES on a PRIOR head must NOT block.""" + reviews = [ + _review(user="cto-founder", state="REQUEST_CHANGES", commit_id=OTHER_HEAD), + ] + _, request_changes = classify_reviews( + reviews, headsha=HEAD, reviewer_set={"alice"} + ) + self.assertEqual(request_changes, []) + def test_latest_review_per_user_wins(self): # alice's REQUEST_CHANGES (latest) supersedes her earlier APPROVED. reviews = [ diff --git a/.gitea/scripts/tests/test_gitea_merge_queue.py b/.gitea/scripts/tests/test_gitea_merge_queue.py index 053c5a9b..be620cb6 100644 --- a/.gitea/scripts/tests/test_gitea_merge_queue.py +++ b/.gitea/scripts/tests/test_gitea_merge_queue.py @@ -317,6 +317,37 @@ def test_genuine_approvals_latest_review_supersedes_earlier(): assert rc == ["agent-reviewer-cr2"] +def test_genuine_approvals_out_of_roster_request_changes_still_blocks(): + """FIX-2 (internal#3210): an official current-head REQUEST_CHANGES from a + login OUTSIDE REVIEWER_SET (e.g. the CTO/founder) must be surfaced in the + request_changes list so the merge is blocked — while roster approvals are + still tallied. The earlier reviewer_set filter dropped it silently.""" + 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"}, + # CTO/founder — NOT in REVIEWER_SET — requests changes on current head. + {"state": "REQUEST_CHANGES", "user": {"login": "hongming-cto"}, + "official": True, "stale": False, "dismissed": False, "commit_id": "HEAD"}, + ] + approvers, rc = mq.genuine_approvals(reviews, headsha="HEAD", reviewer_set=REVIEWERS) + # Roster approvals are still counted (roster gate unchanged for approvers). + assert approvers == {"agent-researcher", "agent-reviewer-cr2"} + # The out-of-roster block is honored. + assert "hongming-cto" in rc + + # End-to-end: that block flows into evaluate_merge_readiness and WAITS, + # even though the 2-genuine approval floor is otherwise satisfied. + decision = mq.evaluate_merge_readiness( + **_ready_kwargs(approvers=approvers, request_changes=rc, required_approvals=2) + ) + assert decision.ready is False + assert decision.action == "wait" + assert "REQUEST_CHANGES" in decision.reason + assert "hongming-cto" in decision.reason + + def test_merge_blocked_when_open_request_changes_on_current_head(): decision = mq.evaluate_merge_readiness( **_ready_kwargs(request_changes=["agent-reviewer-cr2"]) @@ -566,6 +597,114 @@ def test_parse_branch_protection_fail_closed_on_non_object(): mq.parse_branch_protection(None) +# -------------------------------------------------------------------------- +# FIX-1 (internal#3210, CRITICAL fail-open): required_approvals FLOOR. +# +# A degraded `required_approvals` from branch protection — 0 (admin lowered +# it / migration reset / blanked-restored BP), a negative (passes a naive +# `< N`), or a bool True (isinstance(True, int) is True → coerces to 1, +# HALVING a 2-genuine bar) — must NEVER weaken or skip the genuine-approval +# gate. parse_branch_protection clamps UP to REQUIRED_APPROVALS_DEFAULT; +# evaluate_merge_readiness applies an independent floor of 1 on the +# non-exempt path. These tests pin both layers. +# -------------------------------------------------------------------------- + +def _bp_body(required_approvals): + return { + "enable_status_check": True, + "status_check_contexts": ["CI / all-required (pull_request)"], + "required_approvals": required_approvals, + "block_on_rejected_reviews": True, + } + + +def test_parse_branch_protection_clamps_zero_up_to_default(): + """required_approvals: 0 from BP must clamp UP to the default floor (2), + never zero/skip the genuine-approval gate.""" + bp = mq.parse_branch_protection(_bp_body(0)) + assert bp.required_approvals == mq.REQUIRED_APPROVALS_DEFAULT + assert bp.required_approvals >= 1 + + +def test_parse_branch_protection_clamps_negative_up_to_default(): + """A negative required_approvals (would pass a naive `len < N`) must + clamp UP to the default floor, never below it.""" + bp = mq.parse_branch_protection(_bp_body(-1)) + assert bp.required_approvals == mq.REQUIRED_APPROVALS_DEFAULT + + +def test_parse_branch_protection_rejects_bool_true_uses_default(): + """bool True is NOT a valid approval count. isinstance(True, int) is True + in Python, so the naive int() path would coerce True->1 and HALVE a + 2-genuine bar. It must be rejected and fall back to the default floor.""" + bp = mq.parse_branch_protection(_bp_body(True)) + assert bp.required_approvals == mq.REQUIRED_APPROVALS_DEFAULT + # Defensive: a False would also be rejected (treated as invalid). + bp_false = mq.parse_branch_protection(_bp_body(False)) + assert bp_false.required_approvals == mq.REQUIRED_APPROVALS_DEFAULT + + +def test_parse_branch_protection_honors_stricter_value(): + """A BP value ABOVE the default floor is honored as-is (stricter wins).""" + bp = mq.parse_branch_protection(_bp_body(3)) + assert bp.required_approvals == 3 + + +def test_parse_branch_protection_missing_approvals_uses_default(): + """No required_approvals key at all → the default floor, not zero.""" + body = _bp_body(0) + del body["required_approvals"] + bp = mq.parse_branch_protection(body) + assert bp.required_approvals == mq.REQUIRED_APPROVALS_DEFAULT + + +def test_evaluate_merge_readiness_floors_zero_required_approvals_nonexempt(): + """Defence-in-depth: even if a degraded required_approvals=0 reaches + evaluate_merge_readiness on the NON-exempt path, the genuine-approval + gate is NOT skipped — a PR with zero approvers must still WAIT.""" + decision = mq.evaluate_merge_readiness( + **_ready_kwargs( + approvers=set(), + required_approvals=0, + # runtime_bump_exempt omitted → defaults to False (non-exempt) + ) + ) + assert decision.ready is False + assert decision.action == "wait" + assert "insufficient genuine approvals" in decision.reason + # The floor is reported as need >= 1, never 0. + assert "need 1" in decision.reason + + +def test_evaluate_merge_readiness_floors_negative_required_approvals_nonexempt(): + """A negative required_approvals must not pass a naive `len(approvers) < N` + check — the floor forces the gate to require at least 1.""" + decision = mq.evaluate_merge_readiness( + **_ready_kwargs( + approvers=set(), + required_approvals=-5, + ) + ) + assert decision.ready is False + assert decision.action == "wait" + assert "insufficient genuine approvals" in decision.reason + + +def test_evaluate_merge_readiness_floor_does_not_break_runtime_bump_exemption(): + """The non-exempt floor must NOT touch the runtime-bump exemption path, + which legitimately zeroes the HUMAN approval bar (a bot cannot + self-approve). With exempt=True + 0 approvers the PR still merges.""" + decision = mq.evaluate_merge_readiness( + **_ready_kwargs( + approvers=set(), + required_approvals=0, + runtime_bump_exempt=True, + ) + ) + assert decision.ready is True + assert decision.action == "merge" + + # -------------------------------------------------------------------------- # Fix 2: HOL — a permanent merge error HOLDS the PR (applies HOLD_LABEL) # -------------------------------------------------------------------------- -- 2.52.0