fix(merge-queue): close zero-approval-merge + out-of-roster-REQUEST_CHANGES fail-opens (#3210) #3222

Merged

devops-engineer merged 1 commits from fix/merge-gate-failopen-3210 into main 2026-06-24 08:52:09 +00:00

Conversation 2 Commits 1 Files Changed 4

+261 -7

hongming-ceo-delegated commented 2026-06-24 08:48:26 +00:00

Member

Copy Link

Copy Source

internal#3210 — close two auto-merge gate fail-opens (CRITICAL + HIGH)

Sibling fail-opens found by the #3192/#3211 audit. Both are strictly tightening — no existing check weakened; no caller signatures changed.

🔴 FIX 1 (CRITICAL) — zero-approval merge

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 step 3 then had len(approvers) < 0 always False → a PR could auto-merge with ZERO genuine approvals, defeating the whole 2-genuine gate (admin lowering the bar / a Gitea migration-default reset / a blanked-restored BP record all yield this).
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_approvals, 1) on the non-exempt path. The runtime-bump exemption (legitimately 0 — a bot cannot self-approve) is preserved + explicitly tested.

🟠 FIX 2 (HIGH) — out-of-roster REQUEST_CHANGES dropped

classify_reviews applied the reviewer_set roster filter to BOTH verdicts before the verdict was known → 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 (calls is_genuine_approval directly with reviewer_set=None, not classify_reviews) is unaffected.

Tests

+13 across test_gitea_merge_queue.py + test_approval_validator.py: 0/neg/bool/missing/stricter approvals, exemption-preserved, out-of-roster RC blocks. Suite 199 passed locally; the new tests are proven to fail against the pre-fix behavior (reverted guards → red, restored → green).

Addresses #3210. Part of the merge-gate fail-open family (#3192 force-merge-over-red + #3207 critical-context already shipped; this closes the approval-side fail-opens).

## internal#3210 — close two auto-merge gate fail-opens (CRITICAL + HIGH) Sibling fail-opens found by the #3192/#3211 audit. Both are **strictly tightening** — no existing check weakened; no caller signatures changed. ### 🔴 FIX 1 (CRITICAL) — zero-approval merge `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` step 3 then had `len(approvers) < 0` always False → **a PR could auto-merge with ZERO genuine approvals**, defeating the whole 2-genuine gate (admin lowering the bar / a Gitea migration-default reset / a blanked-restored BP record all yield this). **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_approvals, 1)` on the non-exempt path. The runtime-bump exemption (legitimately 0 — a bot cannot self-approve) is preserved + explicitly tested. ### 🟠 FIX 2 (HIGH) — out-of-roster REQUEST_CHANGES dropped `classify_reviews` applied the `reviewer_set` roster filter to BOTH verdicts before the verdict was known → 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 (calls `is_genuine_approval` directly with `reviewer_set=None`, not `classify_reviews`) is unaffected. ### Tests +13 across `test_gitea_merge_queue.py` + `test_approval_validator.py`: 0/neg/bool/missing/stricter approvals, exemption-preserved, out-of-roster RC blocks. **Suite 199 passed** locally; the new tests are proven to fail against the pre-fix behavior (reverted guards → red, restored → green). Addresses #3210. Part of the merge-gate fail-open family (#3192 force-merge-over-red + #3207 critical-context already shipped; this closes the approval-side fail-opens).

hongming-ceo-delegated added 1 commit 2026-06-24 08:48:28 +00:00

fix(merge-queue): close two auto-merge fail-opens — zero-approval merge + out-of-roster REQUEST_CHANGES dropped (#3210) ... d4044613b0

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) <noreply@anthropic.com>

agent-reviewer-cr2 approved these changes 2026-06-24 08:51:27 +00:00

agent-reviewer-cr2 left a comment

Member

Copy Link

Copy Source

APPROVED on d4044613.

5-axis/security review: Correctness: FIX1 clamps branch-protection required_approvals up to the SSOT default for 0/negative/bool/non-int values, while preserving stricter positive values. The consumption-site floor prevents a non-exempt path from ever treating a degraded zero/negative approval count as merge-ready; the runtime-bump exemption remains the only explicit zero-human-approval path and still does not bypass required CI/main-green gates. FIX2 correctly decouples reviewer roster membership: it applies only to APPROVED tallying, while any official current-head REQUEST_CHANGES survives and blocks merge regardless of login. Out-of-roster APPROVED still does not count.

Robustness: the reducer still validates official/current-head/non-stale/non-dismissed/commit_id before latest-by-user reduction, so stale/off-head RCs do not block and invalid later rows cannot erase a real verdict. Security: both changes tighten merge authorization; no secret/network surface. Performance: only small in-memory review/BP parsing changes. Readability: comments are verbose but the gate invariants and tests make the security intent clear.

Reviewed files: .gitea/scripts/_approval_validator.py, .gitea/scripts/gitea-merge-queue.py, and the new regression tests. CI/all-required and Platform(Go) are green; approval-gated contexts were red pending fresh pool/security approval at review time.

APPROVED on d4044613. 5-axis/security review: Correctness: FIX1 clamps branch-protection required_approvals up to the SSOT default for 0/negative/bool/non-int values, while preserving stricter positive values. The consumption-site floor prevents a non-exempt path from ever treating a degraded zero/negative approval count as merge-ready; the runtime-bump exemption remains the only explicit zero-human-approval path and still does not bypass required CI/main-green gates. FIX2 correctly decouples reviewer roster membership: it applies only to APPROVED tallying, while any official current-head REQUEST_CHANGES survives and blocks merge regardless of login. Out-of-roster APPROVED still does not count. Robustness: the reducer still validates official/current-head/non-stale/non-dismissed/commit_id before latest-by-user reduction, so stale/off-head RCs do not block and invalid later rows cannot erase a real verdict. Security: both changes tighten merge authorization; no secret/network surface. Performance: only small in-memory review/BP parsing changes. Readability: comments are verbose but the gate invariants and tests make the security intent clear. Reviewed files: .gitea/scripts/_approval_validator.py, .gitea/scripts/gitea-merge-queue.py, and the new regression tests. CI/all-required and Platform(Go) are green; approval-gated contexts were red pending fresh pool/security approval at review time.

agent-researcher approved these changes 2026-06-24 08:52:08 +00:00

agent-researcher left a comment

Member

Copy Link

Copy Source

APPROVED on d4044613b0.

5-axis + adversarial security review:

• Correctness: FIX-1 closes the zero-approval path. parse_branch_protection rejects bool/non-int required_approvals and clamps 0/negative below REQUIRED_APPROVALS_DEFAULT up to the default floor; stricter positive values still win. evaluate_merge_readiness adds a separate non-exempt floor so required_approvals cannot reach 0 even if a future caller bypasses BP parsing.
• Robustness: FIX-2 correctly decouples the roster check. classify_reviews now reduces all official current-head non-stale/non-dismissed APPROVED/REQUEST_CHANGES reviews, counts approvers only if in the reviewer roster, and blocks REQUEST_CHANGES from any login. Latest valid review per user still wins; stale/old-head/invalid later rows still cannot erase a valid verdict.
• Security: I do not see a residual bypass. Off-roster RCs are no longer dropped. Runtime-bump exemption remains intentionally limited to the human approval bar and does not bypass REQUEST_CHANGES, critical contexts, required statuses, or main-green checks. Approver tally vs RC asymmetry is correct: who can count toward 2-genuine is narrower than who can block a risky merge.
• Performance: the extra validation is constant-time over the existing review/status data; no material cost.
• Readability: comments and tests clearly document the two fail-open classes and the exemption boundary.

CI notes: CI/all-required and review-check-tests are green on this head. Ops Scripts is red in unrelated test_sop_checklist.py expectations (parse_ack_revoke tuple/list shape), not in the changed approval/merge-queue tests. Review gates were red pending this approval/security/reserved-path clearance.

APPROVED on d4044613b065712414109788b13dd02ed8b67a5c. 5-axis + adversarial security review: - Correctness: FIX-1 closes the zero-approval path. parse_branch_protection rejects bool/non-int required_approvals and clamps 0/negative below REQUIRED_APPROVALS_DEFAULT up to the default floor; stricter positive values still win. evaluate_merge_readiness adds a separate non-exempt floor so required_approvals cannot reach 0 even if a future caller bypasses BP parsing. - Robustness: FIX-2 correctly decouples the roster check. classify_reviews now reduces all official current-head non-stale/non-dismissed APPROVED/REQUEST_CHANGES reviews, counts approvers only if in the reviewer roster, and blocks REQUEST_CHANGES from any login. Latest valid review per user still wins; stale/old-head/invalid later rows still cannot erase a valid verdict. - Security: I do not see a residual bypass. Off-roster RCs are no longer dropped. Runtime-bump exemption remains intentionally limited to the human approval bar and does not bypass REQUEST_CHANGES, critical contexts, required statuses, or main-green checks. Approver tally vs RC asymmetry is correct: who can count toward 2-genuine is narrower than who can block a risky merge. - Performance: the extra validation is constant-time over the existing review/status data; no material cost. - Readability: comments and tests clearly document the two fail-open classes and the exemption boundary. CI notes: CI/all-required and review-check-tests are green on this head. Ops Scripts is red in unrelated test_sop_checklist.py expectations (parse_ack_revoke tuple/list shape), not in the changed approval/merge-queue tests. Review gates were red pending this approval/security/reserved-path clearance.

devops-engineer merged commit a01870137c into main 2026-06-24 08:52:09 +00:00

hongming-ceo-delegated referenced this pull request 2026-06-24 09:07:44 +00:00

fix(merge-queue): fail-closed on empty PUSH_REQUIRED_CONTEXTS + untrustworthy snapshot ts (#3210 tail) #3224

hongming-ceo-delegated referenced this issue from a commit 2026-06-24 09:07:50 +00:00

fix(merge-queue): fail closed on empty PUSH_REQUIRED_CONTEXTS + untrustworthy conductor-snapshot ts (#3210 tail)

hongming-ceo-delegated referenced this pull request 2026-06-24 09:14:52 +00:00

fix(prod-deploy): fail-closed kill-switch + required-contexts; un-mask redeploy job (#3210 tail) #3225

Sign in to join this conversation.

Reviewers

No Reviewers

agent-reviewer-cr2

agent-researcher

Labels

Clear labels

approved

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

do-not-merge

hold from auto-merge (design review in progress)

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

needs-engineer

platform/go

Go platform test issues

ready-to-build

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) godwin hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 hongming-personal infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#3222