From e28397e9585ee20272d9a1b3c13df91d85fa194d Mon Sep 17 00:00:00 2001 From: Ferdinando Ametrano Date: Fri, 16 Oct 2026 08:48:22 +0200 Subject: [PATCH] A tree with the Best Practices badge has no bypass actor (issue #1740) In a repository whose README.md carries the bestpractices.dev badge, `main-self-merge` has no bypass actor, so every pull request lands with an approval from somebody other than its author. In an emergency the maintainer adds the bypass back and removes it after the merge. Any other repository keeps the maintainer's bypass in `pull_request` mode. GOVERNANCE.md, CONTRIBUTING.md, ROADMAP.md and README.md (the Scorecard note, sections 11 and 15, the new-repository step) say so. settings_test.py asks a badge tree for no bypass actor and any other for one, reading which tree is which from bestpractices.py's `registered()`. CONTRIBUTING.md is a shared file the other trees copy; the existing EXPECTED_DRIFT entry for it already cites this issue, which stays open until the trees take the text. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Ferdinando Ametrano --- CONTRIBUTING.md | 5 ++-- GOVERNANCE.md | 23 ++++++++++--------- README.md | 52 ++++++++++++++++++++++-------------------- ROADMAP.md | 5 ++-- tests/settings_test.py | 50 +++++++++++++++++++++++++++++++++------- 5 files changed, 87 insertions(+), 48 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ed0c0928..5979a01b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -178,8 +178,9 @@ gh pr merge --squash --match-head-commit With `--auto` added, GitHub runs the same merge once the approval and the checks are in. -**Where [`GOVERNANCE.md`][governance]'s *How a decision is made* allows -it, the maintainer lands without that approval, through the bypass.** +**Where the repository has a bypass, the maintainer may land without +that approval, through it**: [`GOVERNANCE.md`][governance]'s *How a +decision is made* says which repositories. `--admin` turns off `gh`'s own refusal, and GitHub applies the bypass to the merge. `enforce_admins` is off, so it waits for no required check either: diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 32ff7025..e325902a 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -32,14 +32,15 @@ repository. A rule changes by a pull request like any other change. *Branch protection and rulesets* is the rule: every change reaches `main` through one, squashed (section 11's *Signatures* has the signature), and nobody can push to `main` directly. A pull request lands - with an approving review from somebody other than its author. The maintainer - is the only bypass actor, in `pull_request` mode, so the maintainer - can merge a pull request without that approval. In a repository that - publishes releases and whose `README.md` carries the OpenSSF Best - Practices badge (section 2's badge row), the maintainer does so only in - an emergency. In any other repository, the maintainer does so wherever - it helps. The badge's `two_person_review` criterion asks that at least - half of the changes a release ships were reviewed by another person. + with an approving review from somebody other than its author. In a + repository whose `README.md` carries the OpenSSF Best Practices badge + (section 2's badge row), the ruleset has no bypass actor, so nobody + merges without that approval; in an emergency the maintainer adds the + bypass back and removes it after the merge. The badge's + `two_person_review` criterion asks that at least half of the changes a + release ships were reviewed by another person. In any other repository + the maintainer is the only bypass actor, in `pull_request` mode, and + can merge without that approval wherever it helps. `CONTRIBUTING.md`'s *The review* and *Landing it* are the exchange and the landing. - **A disagreement goes to the maintainer.** One that survives a second @@ -62,13 +63,13 @@ repository. A rule changes by a pull request like any other change. Each entry says who holds the role and names the file that says what it owes, rather than restating it. -- **The maintainer** holds the bypass above, and takes the decisions *How - a decision is made* gives the role. The role is held by +- **The maintainer** holds the bypass above where there is one, and takes + the decisions *How a decision is made* gives the role. The role is held by [`fametrano`](https://github.com/fametrano), Ferdinando Ametrano. - **An owner** holds the organization role `admin`, which makes the holder an administrator of every repository of the organization. An administrator can change a repository's rules, so the role is a grant - of trust and not only of access; the bypass stays the maintainer's + of trust and not only of access; a bypass is the maintainer's alone, and a repository's `RELEASING.md` names the three owners as the reviewers of its publishing environments. The owners are [`fametrano`](https://github.com/fametrano), diff --git a/README.md b/README.md index 83fc32da..a774ba2c 100644 --- a/README.md +++ b/README.md @@ -2296,12 +2296,12 @@ and the badge land together. on purpose, to reproduce what a user's own install does.** The dismissal cites the comment the workflow gives for it. - **A check the organization keeps below its maximum on a decision this - file records**: Branch-Protection, held down by the maintainer's - `pull_request`-mode bypass in section 11's *Branch protection and - rulesets*; Code-Review, held down by merges through it landing - with no second person's approval, a bot's review not counting, as - section 11's *Review* says; and Fuzzing on `bitcoin-core-rpc`, which - the `fuzz` entry below already answers. + file records**: Branch-Protection, held down by `enforce_admins` off and + one approving review, as section 11's *Branch protection and + rulesets* records; Code-Review, held down by a bot's review not + counting, as section 11's *Review* says, and by merges through the + bypass in the trees that keep it; and Fuzzing on + `bitcoin-core-rpc`, which the `fuzz` entry below already answers. A sub-maximum score outside these three classes is still an issue. @@ -2717,28 +2717,32 @@ and classic protection, taking the most restrictive combination: pushes, no deletions. **No bypass actor, for anyone, ever.** - **`main-self-merge`** — require a pull request, one approving review, dismissal of stale reviews, conversation resolution, and `squash` as - the only merge method it accepts. Bypass: the maintainer, in - **`pull_request` mode**. + the only merge method it accepts. **No bypass actor** in a tree whose + `README.md` carries the OpenSSF Best Practices badge; in any other + tree, the maintainer, in **`pull_request` mode**. - **`tag-integrity`**, target `tag`, `refs/tags/v*` — required signatures and nothing else, so the recovery path that deletes and re-tags a failed release still works. -**The bypass mode is the whole of the design.** `pull_request` excuses -its holder *while merging a pull request* and at no other time, which -is all the bypass is for: a pull request landing without anybody -else's approval. `GOVERNANCE.md`'s *How a decision is made* says when -the maintainer uses it. A direct push to `main` is refused for everyone; -`always` would permit one, and the signature check reports after a push -and refuses nothing. +`GOVERNANCE.md`'s *How a decision is made* says what a tree with the +badge does in an emergency. -**What it excuses is the rule, not the approval count**, and +**In a tree without the badge, the bypass mode is the whole of the +design.** `pull_request` excuses its holder *while merging a pull +request* and at no other time, which is all the bypass is for: a pull +request landing without anybody else's approval. A direct push to `main` +is refused for everyone; `always` would permit one, and the signature +check reports after a push and refuses nothing. + +**What the bypass excuses is the rule, not the approval count**, and `dismiss_stale_reviews_on_push` is a parameter of that same rule. So the dismissal binds every merge except one through the bypass, and the `--match-head-commit` on `CONTRIBUTING.md`'s merge through the bypass is what refuses a moved head there. -Two settings hold the door, not one: the classic review requirement is -cleared for the maintainer by `enforce_admins: false` *plus* admin, and +Where the bypass stays, two settings hold the door, not one: the classic +review requirement is cleared for the maintainer by `enforce_admins: false` +*plus* admin, and turning `enforce_admins` on would deadlock every solo merge. That setting clears the whole of classic protection for that account. @@ -2942,10 +2946,6 @@ job's verification step reads, is the one place the verdict lives. approval**, and it does not buy the OpenSSF Scorecard's `Code-Review` check, which does not count a review by a bot as code review. -**btclib-org/.github#341 holds the removal of the ruleset's -`bypass_actors`**, which would put a person's approval on the critical -path of every merge. - **The ack of record is `claude-review.yml`'s**, and an author's own is not one: a comment from the account that opened the pull request says its gates were run, and is not a reading. What triggers the workflow is @@ -4019,8 +4019,9 @@ outside it is correct where the reason is beside it. The settings: squash the only method, the `main-integrity` rules held with an **empty** bypass list, `required_signatures` **off** in classic protection and -in every ruleset over `main`, the self-merge -bypass in `pull_request` mode and never `always`, a token that is `read`, +in every ruleset over `main`, the `main-self-merge` +bypass list empty in a tree with the badge and, in any other, one actor +in `pull_request` mode and never `always`, a token that is `read`, and `allowed_actions: all` with `sha_pinning_required: true`: ```shell @@ -4531,7 +4532,8 @@ No tool checks them. squash-only, with auto-merge; `delete_branch_on_merge`; the three rulesets — only after that push, since section 11's bypass mode refuses a direct one once `main-self-merge` is active, and the root - commit is necessarily one; classic protection with the required + commit is necessarily one; the bypass is the maintainer's until the + `README.md` carries the badge; classic protection with the required checks bound to the Actions app; the publishing environments; the read-only default token; secret scanning, its push protection and Dependabot security updates; private vulnerability reporting; the diff --git a/ROADMAP.md b/ROADMAP.md index 3f7bf933..606931cc 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -76,5 +76,6 @@ not deadlines. - **A pinned toolchain for the macOS and Windows wheels of `btclib-secp256k1`** (btclib-org/btclib-secp256k1#554): section 12's *The compiled wheels are outside that property* says why. -- **Removing the maintainer's bypass from the branch rules**, until the - maintainer decides otherwise (btclib-org/.github#341). +- **Removing the maintainer's bypass from the branch rules of the trees + without the Best Practices badge**, until the maintainer decides + otherwise. diff --git a/tests/settings_test.py b/tests/settings_test.py index 90966274..d88eb4c2 100644 --- a/tests/settings_test.py +++ b/tests/settings_test.py @@ -18,6 +18,9 @@ from __future__ import annotations +import importlib.util +import sys +from pathlib import Path from typing import Any import pytest @@ -29,6 +32,29 @@ pytestmark = pytest.mark.integration +_SCRIPT = Path(__file__).parent.parent / ".github" / "scripts" / "bestpractices.py" + + +@pytest.fixture(scope="session") +def badge_trees() -> set[str]: + """Return the trees whose README carries a bestpractices.dev badge. + + Read by the script that registers them, so that this suite and the + saved answers agree on which trees these are. + + :returns: their names. + """ + spec = importlib.util.spec_from_file_location("bestpractices", _SCRIPT) + assert spec is not None + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules["bestpractices"] = module + try: + spec.loader.exec_module(module) + finally: + del sys.modules["bestpractices"] + return set(module.registered()) + @pytest.fixture(scope="session") def rulesets(repositories: list[str]) -> dict[str, list[dict[str, Any]]]: @@ -163,24 +189,29 @@ def test_main_refuses_a_rewritten_commit( ) -def test_a_merge_needs_one_review_and_one_person_may_merge_anyway( +def test_a_merge_needs_one_review_and_a_badge_tree_has_no_bypass( rulesets: dict[str, list[dict[str, Any]]], + badge_trees: set[str], ) -> None: - """Review is the gate, and the bypass excuses it while merging only. + """Review is the gate; a badge tree has no bypass, any other has one. - `pull_request` as a bypass mode excuses its holder from the review - rule while merging a pull request and at no other time. `always` - would permit a direct push, which is the thing the integrity rules - above are for. + A tree whose README carries the bestpractices.dev badge has no bypass + actor on the review rule, so every merge needs an approval from + somebody other than its author. Any other tree has exactly one actor, + in `pull_request` mode, which excuses its holder from the review rule + while merging a pull request and at no other time. `always` would + permit a direct push, which is the thing the integrity rules above + are for. One ruleset carries it, because two would each name their own approval count and their own bypass list and the branch would enforce whichever is stricter -- an answer nobody can read off either page. That the rule exists and how many approvals it asks for is public; - the mode is not, for the reason the test above gives. + the bypass list is not, for the reason the test above gives. :param rulesets: the active rulesets of each repository. + :param badge_trees: the trees whose README carries the badge. """ wrong: dict[str, str] = {} unseen: list[str] = [] @@ -202,7 +233,10 @@ def test_a_merge_needs_one_review_and_one_person_may_merge_anyway( unseen.append(repository) continue modes = sorted(actor["bypass_mode"] for actor in actors) - if modes != ["pull_request"]: + if repository in badge_trees: + if modes: + wrong[repository] = f"bypass actors are {modes}, not none" + elif modes != ["pull_request"]: wrong[repository] = f"bypass actors are {modes}, not one merging one" assert not wrong, f"repositories whose review rule is not the design: {wrong}" if unseen: