A tree with the Best Practices badge has no bypass actor (issue #1740)

CONTRIBUTING.md

@@ -178,8 +178,9 @@ gh pr merge <n> --squash --match-head-commit <the head the checks ran on>
 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:

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

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

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.

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: