.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)

.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",

.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 = [

.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)
 # --------------------------------------------------------------------------