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: