From d30030d4279f7b254d2af7a973531767af3e8195 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Tue, 20 Oct 2026 01:02:33 +0200 Subject: [PATCH 1/4] ci: require the allow-list approval check, with a maintainer override Make 'approval' a required status check on main, so an allow-list PR cannot merge unapproved (#1409 did, as the check was advisory). A maintainer can waive it with the allowlist-approval-override label, for their own allow-list PRs; it counts only if whoever last added it has write access. Generated-by: Claude Opus 5 --- .asf.yaml | 4 ++ .github/workflows/allowlist_approval.yml | 12 +++--- README.md | 2 +- scripts/check_allowlist_approval.py | 49 +++++++++++++++++++++++- scripts/test_check_allowlist_approval.py | 36 +++++++++++++++++ 5 files changed, 96 insertions(+), 7 deletions(-) diff --git a/.asf.yaml b/.asf.yaml index f6264b23..00d30437 100644 --- a/.asf.yaml +++ b/.asf.yaml @@ -25,6 +25,10 @@ github: required_status_checks: # strict means "Require branches to be up to date before merging". strict: false + contexts: + # allowlist_approval.yml: an allow-list PR needs a maintainer's + # approval or override label; other PRs pass it at once. + - approval notifications: commits: commits@infra.apache.org diff --git a/.github/workflows/allowlist_approval.yml b/.github/workflows/allowlist_approval.yml index b472bf7c..6403acd8 100644 --- a/.github/workflows/allowlist_approval.yml +++ b/.github/workflows/allowlist_approval.yml @@ -18,22 +18,24 @@ # # Fails PRs that touch the allow list (actions.yml, approved_patterns.yml or # the generated Dependabot composite action) until at least one maintainer — -# a reviewer with write access — has approved. Other PRs pass immediately, -# so the check can be required on every PR. +# a reviewer with write access — has approved, or a maintainer has waived +# that with the allowlist-approval-override label. Other PRs pass +# immediately, so the check is required on every PR (.asf.yaml). # -# Re-runs on review events so the status flips as soon as an approval lands -# or is dismissed. +# Re-runs on review and label events so the status flips as soon as an +# approval or the override lands or goes. name: Allowlist Maintainer Approval on: pull_request: - types: [opened, synchronize, reopened] + types: [opened, synchronize, reopened, labeled, unlabeled] pull_request_review: types: [submitted, edited, dismissed] permissions: contents: read pull-requests: read + issues: read # who added the override label concurrency: group: allowlist-approval-${{ github.event.pull_request.number }} diff --git a/README.md b/README.md index b8b76e56..dbfb4ad2 100644 --- a/README.md +++ b/README.md @@ -355,7 +355,7 @@ The workflows below run on PRs that touch the allow list, so the verification an - **`verify` job in `verify_manual_action.yml`** — triggers on human-authored PRs that modify `actions.yml` or `approved_patterns.yml` (i.e. manual allow-list additions / version bumps). Dependabot-authored PRs are skipped, since they are already covered by the workflow above. - **`check_action_tags` job in `check_action_tags.yml`** — triggers when `actions.yml`, `approved_patterns.yml`, the generated Dependabot composite action, the update workflow, or gateway verification code changes. It verifies that configured action SHAs exist and, when a `tag` is recorded, that the SHA is reachable from that Git tag or branch. -- **`approval` job in `allowlist_approval.yml`** — runs on every PR and on every review submitted, edited or dismissed. If the PR touches `actions.yml`, `approved_patterns.yml` or the generated Dependabot composite action, it fails until at least one maintainer (a reviewer with write access to this repository) has approved; only each reviewer's latest approve / request-changes / dismissal counts. Other PRs pass immediately. The logic lives in `scripts/check_allowlist_approval.py`, which the job runs from the PR's base commit so a PR cannot rewrite its own gate. +- **`approval` job in `allowlist_approval.yml`** — runs on every PR, on every review submitted, edited or dismissed, and on label changes. If the PR touches `actions.yml`, `approved_patterns.yml` or the generated Dependabot composite action, it fails until at least one maintainer (a reviewer with write access to this repository) has approved; only each reviewer's latest approve / request-changes / dismissal counts. A maintainer can waive the approval, for example on an allow-list PR they authored and so cannot approve, by adding the `allowlist-approval-override` label; it counts only while on the PR and only if whoever last added it has write access. Other PRs pass immediately. The check is a required status check on `main` (`.asf.yaml`), so an allow-list PR cannot be merged without one or the other. The logic lives in `scripts/check_allowlist_approval.py`, which the job runs from the PR's base commit so a PR cannot rewrite its own gate. These workflows use regular `pull_request` (and `pull_request_review`) triggers with read-only permissions and no PR comments — pass/fail is surfaced through the status check. They do not auto-approve or merge; a human reviewer must still approve. diff --git a/scripts/check_allowlist_approval.py b/scripts/check_allowlist_approval.py index 0e116452..99e32483 100644 --- a/scripts/check_allowlist_approval.py +++ b/scripts/check_allowlist_approval.py @@ -32,6 +32,11 @@ repository. Only each reviewer's latest decisive review counts, so an approval later followed by "changes requested" or a dismissal does not. +A maintainer can waive the approval by adding ``OVERRIDE_LABEL`` -- for an +allow-list PR they authored themselves, which GitHub does not let them +approve. The label counts only while it is on the PR and whoever last added +it has write access: triagers can add labels too. + The decision logic is kept in pure functions for unit testing; ``main`` fetches the PR files, reviews and reviewer permissions through ``gh api``. @@ -59,6 +64,8 @@ # ``maintain`` is reported as ``write`` there. MAINTAINER_PERMISSIONS = frozenset({"admin", "maintain", "write"}) +OVERRIDE_LABEL = "allowlist-approval-override" + # Review states that set a reviewer's verdict. COMMENTED and PENDING do not # change it, matching how GitHub itself counts approvals. DECISIVE_STATES = frozenset({"APPROVED", "CHANGES_REQUESTED", "DISMISSED"}) @@ -90,6 +97,29 @@ def maintainer_approvers( ] +def override_by( + labels: Iterable[str], + label_events: Iterable[dict], + permission_of: Callable[[str], str], +) -> str | None: + """The maintainer whose override label waives the approval, or None. + + ``label_events`` must be in chronological order, as the API returns them. + A label with no visible ``labeled`` event counts for nothing: who added it + is unknown, and unknown must not be more permissive than a refusal. + """ + if OVERRIDE_LABEL not in labels: + return None + added = [ + e["login"] + for e in label_events + if e["event"] == "labeled" and e["label"] == OVERRIDE_LABEL + ] + if not added: + return None + return added[-1] if permission_of(added[-1]) in MAINTAINER_PERMISSIONS else None + + def _gh_lines(*args: str) -> list[str]: out = subprocess.run( ["gh", "api", *args], check=True, capture_output=True, text=True @@ -148,9 +178,26 @@ def main(argv: list[str] | None = None) -> int: if approved_by: print(f"Allow-list PR approved by maintainer(s): {', '.join(approved_by)}") return 0 + + labels = _gh_lines(f"repos/{args.repo}/issues/{args.pr}", "--jq", ".labels[].name") + label_events = [ + json.loads(line) + for line in _gh_lines( + f"repos/{args.repo}/issues/{args.pr}/events", + "--paginate", + "--jq", + '.[] | select(.event == "labeled" or .event == "unlabeled")' + " | {event, label: .label.name, login: .actor.login}", + ) + ] + waived_by = override_by(labels, label_events, lambda login: _permission_of(args.repo, login)) + if waived_by: + print(f"Maintainer approval waived by {waived_by} with the {OVERRIDE_LABEL} label.") + return 0 print( "::error::Allow-list PR has no approval from a maintainer " - "(a reviewer with write access to the repository)." + "(a reviewer with write access to the repository). A maintainer can " + f"waive it with the {OVERRIDE_LABEL} label." ) return 1 diff --git a/scripts/test_check_allowlist_approval.py b/scripts/test_check_allowlist_approval.py index 958fe847..6008ba1a 100644 --- a/scripts/test_check_allowlist_approval.py +++ b/scripts/test_check_allowlist_approval.py @@ -79,3 +79,39 @@ def test_no_maintainer_approval(): reviews = [_review("carol", "APPROVED"), _review("alice", "COMMENTED")] permissions = {"alice": "admin", "carol": "read"} assert caa.maintainer_approvers(reviews, permissions.__getitem__) == [] + + +def _labeled(login, label=None, event="labeled"): + return {"event": event, "label": label or caa.OVERRIDE_LABEL, "login": login} + + +def _permission(**perms): + return lambda login: perms.get(login, "read") + + +def test_override_by_a_maintainer_waives_approval(): + perms = _permission(alice="write") + assert caa.override_by([caa.OVERRIDE_LABEL], [_labeled("alice")], perms) == "alice" + + +def test_override_needs_the_label_on_the_pr(): + perms = _permission(alice="write") + events = [_labeled("alice"), _labeled("alice", event="unlabeled")] + assert caa.override_by([], events, perms) is None + + +def test_override_by_a_triager_does_not_count(): + perms = _permission(alice="write", tri="triage") + # A maintainer added it once, but whoever added it last is a triager. + events = [_labeled("alice"), _labeled("alice", event="unlabeled"), _labeled("tri")] + assert caa.override_by([caa.OVERRIDE_LABEL], events, perms) is None + + +def test_override_with_no_labeled_event_does_not_count(): + assert caa.override_by([caa.OVERRIDE_LABEL], [], _permission()) is None + + +def test_other_labels_are_not_an_override(): + perms = _permission(alice="write") + assert caa.override_by(["preview"], [_labeled("alice", label="preview")], perms) is None + From 52d2479334ea8a19408ce8157bfefc228a1cf5d7 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Tue, 20 Oct 2026 01:06:14 +0200 Subject: [PATCH 2/4] ci: require verify too, passing at once on non-allow-list PRs Both verify workflows now start on every PR so the check can be required; a first step decides from the PR's file list and skips the rest when the allow list is untouched, failing closed. Generated-by: Claude Opus 5 --- .asf.yaml | 3 ++ .../workflows/verify_dependabot_action.yml | 35 +++++++++++++++++- .github/workflows/verify_manual_action.yml | 37 +++++++++++++++++-- README.md | 2 + 4 files changed, 72 insertions(+), 5 deletions(-) diff --git a/.asf.yaml b/.asf.yaml index 00d30437..9f501c19 100644 --- a/.asf.yaml +++ b/.asf.yaml @@ -29,6 +29,9 @@ github: # allowlist_approval.yml: an allow-list PR needs a maintainer's # approval or override label; other PRs pass it at once. - approval + # verify_*_action.yml: rebuilds an allow-list PR's action and + # compares it with the published one; other PRs pass it at once. + - verify notifications: commits: commits@infra.apache.org diff --git a/.github/workflows/verify_dependabot_action.yml b/.github/workflows/verify_dependabot_action.yml index 8bc101c5..00bcdb15 100644 --- a/.github/workflows/verify_dependabot_action.yml +++ b/.github/workflows/verify_dependabot_action.yml @@ -20,8 +20,6 @@ name: Verify Dependabot Action Build on: pull_request: - paths: - - .github/actions/for-dependabot-triggered-reviews/action.yml permissions: contents: read @@ -32,13 +30,46 @@ jobs: if: github.event.pull_request.user.login == 'dependabot[bot]' runs-on: ubuntu-26.04 steps: + # Required on every PR (.asf.yaml), so it runs on every PR, but only an + # allow-list PR is verified: for any other PR this step finds nothing to + # do and every step after it is skipped. Deciding here, not in a job + # this one needs, keeps it fail-closed: a failed lookup fails `verify`, + # where a failed upstream job would only skip it, and a skipped job + # passes a required check. A file list the API may have cut short (it + # stops at 3000) is verified, not skipped. + - name: Does this PR touch the allow list? + id: scope + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + WATCHED: | + .github/actions/for-dependabot-triggered-reviews/action.yml + run: | + files="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate \ + --jq '.[] | .filename, (.previous_filename // empty)')" + listed="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' | wc -l)" + changed="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.changed_files')" + if [ "${listed}" -lt "${changed}" ]; then + echo "Only ${listed} of ${changed} changed files listed; verifying to be safe." + echo "run=true" >> "$GITHUB_OUTPUT" + elif grep -Fxq -f <(printf '%s' "${WATCHED}" | sed '/^$/d') <<< "${files}"; then + echo "run=true" >> "$GITHUB_OUTPUT" + else + echo "Not an allow-list PR; nothing to verify." + echo "run=false" >> "$GITHUB_OUTPUT" + fi + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + if: steps.scope.outputs.run == 'true' with: persist-credentials: false - run: pipx install uv + if: steps.scope.outputs.run == 'true' - name: Verify action build + if: steps.scope.outputs.run == 'true' run: | # Disable workflow command processing so that strings like # "##[add-matcher]" or "::error::" in action diffs are not diff --git a/.github/workflows/verify_manual_action.yml b/.github/workflows/verify_manual_action.yml index e719552a..d9961409 100644 --- a/.github/workflows/verify_manual_action.yml +++ b/.github/workflows/verify_manual_action.yml @@ -26,9 +26,6 @@ name: Verify Manual Allowlist PR on: pull_request: - paths: - - actions.yml - - approved_patterns.yml permissions: contents: read @@ -39,13 +36,47 @@ jobs: if: github.event.pull_request.user.login != 'dependabot[bot]' runs-on: ubuntu-26.04 steps: + # Required on every PR (.asf.yaml), so it runs on every PR, but only an + # allow-list PR is verified: for any other PR this step finds nothing to + # do and every step after it is skipped. Deciding here, not in a job + # this one needs, keeps it fail-closed: a failed lookup fails `verify`, + # where a failed upstream job would only skip it, and a skipped job + # passes a required check. A file list the API may have cut short (it + # stops at 3000) is verified, not skipped. + - name: Does this PR touch the allow list? + id: scope + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + WATCHED: | + actions.yml + approved_patterns.yml + run: | + files="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate \ + --jq '.[] | .filename, (.previous_filename // empty)')" + listed="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' | wc -l)" + changed="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.changed_files')" + if [ "${listed}" -lt "${changed}" ]; then + echo "Only ${listed} of ${changed} changed files listed; verifying to be safe." + echo "run=true" >> "$GITHUB_OUTPUT" + elif grep -Fxq -f <(printf '%s' "${WATCHED}" | sed '/^$/d') <<< "${files}"; then + echo "run=true" >> "$GITHUB_OUTPUT" + else + echo "Not an allow-list PR; nothing to verify." + echo "run=false" >> "$GITHUB_OUTPUT" + fi + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + if: steps.scope.outputs.run == 'true' with: persist-credentials: false - run: pipx install uv + if: steps.scope.outputs.run == 'true' - name: Verify action build + if: steps.scope.outputs.run == 'true' run: | # Disable workflow command processing so that strings like # "##[add-matcher]" or "::error::" in action diffs are not diff --git a/README.md b/README.md index dbfb4ad2..04b60f73 100644 --- a/README.md +++ b/README.md @@ -353,6 +353,8 @@ The workflows below run on PRs that touch the allow list, so the verification an - **`verify` job in `verify_dependabot_action.yml`** — triggers on Dependabot PRs that modify `.github/actions/for-dependabot-triggered-reviews/action.yml`. Extracts the action reference from the PR, rebuilds the compiled JavaScript in Docker, and compares it against the published version. - **`verify` job in `verify_manual_action.yml`** — triggers on human-authored PRs that modify `actions.yml` or `approved_patterns.yml` (i.e. manual allow-list additions / version bumps). Dependabot-authored PRs are skipped, since they are already covered by the workflow above. + + Both `verify` jobs start on every PR, because `verify` is a required status check on `main` (`.asf.yaml`) and a check that never starts would block a PR forever. Their first step asks the API for the PR's changed files; a PR that touches none of the files above passes at once, with no checkout or build. The decision is made inside the job, so a failed lookup fails `verify` rather than skipping it, and a file list the API may have cut short is verified rather than skipped. - **`check_action_tags` job in `check_action_tags.yml`** — triggers when `actions.yml`, `approved_patterns.yml`, the generated Dependabot composite action, the update workflow, or gateway verification code changes. It verifies that configured action SHAs exist and, when a `tag` is recorded, that the SHA is reachable from that Git tag or branch. - **`approval` job in `allowlist_approval.yml`** — runs on every PR, on every review submitted, edited or dismissed, and on label changes. If the PR touches `actions.yml`, `approved_patterns.yml` or the generated Dependabot composite action, it fails until at least one maintainer (a reviewer with write access to this repository) has approved; only each reviewer's latest approve / request-changes / dismissal counts. A maintainer can waive the approval, for example on an allow-list PR they authored and so cannot approve, by adding the `allowlist-approval-override` label; it counts only while on the PR and only if whoever last added it has write access. Other PRs pass immediately. The check is a required status check on `main` (`.asf.yaml`), so an allow-list PR cannot be merged without one or the other. The logic lives in `scripts/check_allowlist_approval.py`, which the job runs from the PR's base commit so a PR cannot rewrite its own gate. From c6d11e8fa8d307de4243393bb130760d6d3b149a Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Tue, 20 Oct 2026 01:08:25 +0200 Subject: [PATCH 3/4] ci: verify every allow-list file, compare names exactly Both verify jobs watch all three allow-list paths, so the one that runs for a PR's author covers each; file names are matched as JSON strings and counted as entries, and a list at the API cap is verified. Generated-by: Claude Opus 5 --- .../workflows/verify_dependabot_action.yml | 19 ++++++++++++++----- .github/workflows/verify_manual_action.yml | 18 +++++++++++++----- README.md | 2 +- 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/.github/workflows/verify_dependabot_action.yml b/.github/workflows/verify_dependabot_action.yml index 00bcdb15..1046259e 100644 --- a/.github/workflows/verify_dependabot_action.yml +++ b/.github/workflows/verify_dependabot_action.yml @@ -43,17 +43,26 @@ jobs: GH_TOKEN: ${{ github.token }} REPO: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} + # The whole allow list, as check_allowlist_approval.py has it, in + # both verify workflows: each job runs for one kind of author, so + # the one that runs must watch every allow-list file. WATCHED: | + actions.yml + approved_patterns.yml .github/actions/for-dependabot-triggered-reviews/action.yml run: | + # Names compared as JSON strings, one per line, so a file name with + # a newline in it can neither hide a match nor fake one. Entries + # are counted, not lines, for the same reason. files="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate \ - --jq '.[] | .filename, (.previous_filename // empty)')" - listed="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' | wc -l)" + --jq '.[] | (.filename, (.previous_filename // empty)) | @json')" + listed="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[] | 1' | wc -l)" changed="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.changed_files')" - if [ "${listed}" -lt "${changed}" ]; then - echo "Only ${listed} of ${changed} changed files listed; verifying to be safe." + watched="$(printf '%s' "${WATCHED}" | sed '/^$/d' | jq -R '.')" + if [ "${listed}" -lt "${changed}" ] || [ "${changed}" -ge 3000 ]; then + echo "The file list may be incomplete (${listed} of ${changed}); verifying to be safe." echo "run=true" >> "$GITHUB_OUTPUT" - elif grep -Fxq -f <(printf '%s' "${WATCHED}" | sed '/^$/d') <<< "${files}"; then + elif grep -Fxq -f <(printf '%s\n' "${watched}") <<< "${files}"; then echo "run=true" >> "$GITHUB_OUTPUT" else echo "Not an allow-list PR; nothing to verify." diff --git a/.github/workflows/verify_manual_action.yml b/.github/workflows/verify_manual_action.yml index d9961409..280e5546 100644 --- a/.github/workflows/verify_manual_action.yml +++ b/.github/workflows/verify_manual_action.yml @@ -49,18 +49,26 @@ jobs: GH_TOKEN: ${{ github.token }} REPO: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} + # The whole allow list, as check_allowlist_approval.py has it, in + # both verify workflows: each job runs for one kind of author, so + # the one that runs must watch every allow-list file. WATCHED: | actions.yml approved_patterns.yml + .github/actions/for-dependabot-triggered-reviews/action.yml run: | + # Names compared as JSON strings, one per line, so a file name with + # a newline in it can neither hide a match nor fake one. Entries + # are counted, not lines, for the same reason. files="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate \ - --jq '.[] | .filename, (.previous_filename // empty)')" - listed="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' | wc -l)" + --jq '.[] | (.filename, (.previous_filename // empty)) | @json')" + listed="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[] | 1' | wc -l)" changed="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.changed_files')" - if [ "${listed}" -lt "${changed}" ]; then - echo "Only ${listed} of ${changed} changed files listed; verifying to be safe." + watched="$(printf '%s' "${WATCHED}" | sed '/^$/d' | jq -R '.')" + if [ "${listed}" -lt "${changed}" ] || [ "${changed}" -ge 3000 ]; then + echo "The file list may be incomplete (${listed} of ${changed}); verifying to be safe." echo "run=true" >> "$GITHUB_OUTPUT" - elif grep -Fxq -f <(printf '%s' "${WATCHED}" | sed '/^$/d') <<< "${files}"; then + elif grep -Fxq -f <(printf '%s\n' "${watched}") <<< "${files}"; then echo "run=true" >> "$GITHUB_OUTPUT" else echo "Not an allow-list PR; nothing to verify." diff --git a/README.md b/README.md index 04b60f73..a842655e 100644 --- a/README.md +++ b/README.md @@ -354,7 +354,7 @@ The workflows below run on PRs that touch the allow list, so the verification an - **`verify` job in `verify_dependabot_action.yml`** — triggers on Dependabot PRs that modify `.github/actions/for-dependabot-triggered-reviews/action.yml`. Extracts the action reference from the PR, rebuilds the compiled JavaScript in Docker, and compares it against the published version. - **`verify` job in `verify_manual_action.yml`** — triggers on human-authored PRs that modify `actions.yml` or `approved_patterns.yml` (i.e. manual allow-list additions / version bumps). Dependabot-authored PRs are skipped, since they are already covered by the workflow above. - Both `verify` jobs start on every PR, because `verify` is a required status check on `main` (`.asf.yaml`) and a check that never starts would block a PR forever. Their first step asks the API for the PR's changed files; a PR that touches none of the files above passes at once, with no checkout or build. The decision is made inside the job, so a failed lookup fails `verify` rather than skipping it, and a file list the API may have cut short is verified rather than skipped. + Both `verify` jobs start on every PR, because `verify` is a required status check on `main` (`.asf.yaml`) and a check that never starts would block a PR forever. Their first step asks the API for the PR's changed files; a PR that touches none of the three allow-list files (`actions.yml`, `approved_patterns.yml`, the Dependabot composite action) passes at once, with no checkout or build. Both jobs watch all three, so whichever runs for the PR's author covers every allow-list file. The decision is made inside the job, so a failed lookup fails `verify` rather than skipping it, and a file list the API may have cut short is verified rather than skipped. - **`check_action_tags` job in `check_action_tags.yml`** — triggers when `actions.yml`, `approved_patterns.yml`, the generated Dependabot composite action, the update workflow, or gateway verification code changes. It verifies that configured action SHAs exist and, when a `tag` is recorded, that the SHA is reachable from that Git tag or branch. - **`approval` job in `allowlist_approval.yml`** — runs on every PR, on every review submitted, edited or dismissed, and on label changes. If the PR touches `actions.yml`, `approved_patterns.yml` or the generated Dependabot composite action, it fails until at least one maintainer (a reviewer with write access to this repository) has approved; only each reviewer's latest approve / request-changes / dismissal counts. A maintainer can waive the approval, for example on an allow-list PR they authored and so cannot approve, by adding the `allowlist-approval-override` label; it counts only while on the PR and only if whoever last added it has write access. Other PRs pass immediately. The check is a required status check on `main` (`.asf.yaml`), so an allow-list PR cannot be merged without one or the other. The logic lives in `scripts/check_allowlist_approval.py`, which the job runs from the PR's base commit so a PR cannot rewrite its own gate. From 4bbf85f5d65c8bfb69f9fe37b490ef135157e09b Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Tue, 20 Oct 2026 01:16:53 +0200 Subject: [PATCH 4/4] ci: put the required checks in a ruleset; drop the label override Only a ruleset can say who may bypass a required check: admins always (asfgit's direct sync pushes), write access on pull requests only. That replaces the allowlist-approval-override label, so the approval script is back to main's version. Generated-by: Claude Opus 5 --- .asf.yaml | 42 ++++++++++++++++---- .github/workflows/allowlist_approval.yml | 10 ++--- README.md | 4 +- scripts/check_allowlist_approval.py | 49 +----------------------- scripts/test_check_allowlist_approval.py | 36 ----------------- 5 files changed, 42 insertions(+), 99 deletions(-) diff --git a/.asf.yaml b/.asf.yaml index 9f501c19..bc83dfbf 100644 --- a/.asf.yaml +++ b/.asf.yaml @@ -25,13 +25,41 @@ github: required_status_checks: # strict means "Require branches to be up to date before merging". strict: false - contexts: - # allowlist_approval.yml: an allow-list PR needs a maintainer's - # approval or override label; other PRs pass it at once. - - approval - # verify_*_action.yml: rebuilds an allow-list PR's action and - # compares it with the published one; other PRs pass it at once. - - verify + # The checks every PR must pass to merge. A ruleset rather than + # protected_branches, because only a ruleset can name who may bypass it. + rulesets: + - name: "Required checks" + target: branch + enforcement: active + conditions: + ref_name: + include: + - "~DEFAULT_BRANCH" + exclude: [] + bypass_actors: + # Repository admins, always: update.yml pushes its sync commit to + # main directly as asfgit, which is an admin. + - actor_id: 5 + actor_type: RepositoryRole + bypass_mode: always + # Write access, on pull requests only: a maintainer can choose to + # merge past the checks, never push past them. + - actor_id: 4 + actor_type: RepositoryRole + bypass_mode: pull_request + rules: + - type: required_status_checks + parameters: + strict_required_status_checks_policy: false + required_status_checks: + # allowlist_approval.yml: an allow-list PR needs a maintainer's + # approval; other PRs pass it at once. + - context: approval + integration_id: -1 + # verify_*_action.yml: rebuilds an allow-list PR's action and + # compares it with the published one; other PRs pass it at once. + - context: verify + integration_id: -1 notifications: commits: commits@infra.apache.org diff --git a/.github/workflows/allowlist_approval.yml b/.github/workflows/allowlist_approval.yml index 6403acd8..b9bc13b0 100644 --- a/.github/workflows/allowlist_approval.yml +++ b/.github/workflows/allowlist_approval.yml @@ -18,24 +18,22 @@ # # Fails PRs that touch the allow list (actions.yml, approved_patterns.yml or # the generated Dependabot composite action) until at least one maintainer — -# a reviewer with write access — has approved, or a maintainer has waived -# that with the allowlist-approval-override label. Other PRs pass +# a reviewer with write access — has approved. Other PRs pass # immediately, so the check is required on every PR (.asf.yaml). # -# Re-runs on review and label events so the status flips as soon as an -# approval or the override lands or goes. +# Re-runs on review events so the status flips as soon as an approval lands +# or is dismissed. name: Allowlist Maintainer Approval on: pull_request: - types: [opened, synchronize, reopened, labeled, unlabeled] + types: [opened, synchronize, reopened] pull_request_review: types: [submitted, edited, dismissed] permissions: contents: read pull-requests: read - issues: read # who added the override label concurrency: group: allowlist-approval-${{ github.event.pull_request.number }} diff --git a/README.md b/README.md index a842655e..47b3f103 100644 --- a/README.md +++ b/README.md @@ -354,10 +354,10 @@ The workflows below run on PRs that touch the allow list, so the verification an - **`verify` job in `verify_dependabot_action.yml`** — triggers on Dependabot PRs that modify `.github/actions/for-dependabot-triggered-reviews/action.yml`. Extracts the action reference from the PR, rebuilds the compiled JavaScript in Docker, and compares it against the published version. - **`verify` job in `verify_manual_action.yml`** — triggers on human-authored PRs that modify `actions.yml` or `approved_patterns.yml` (i.e. manual allow-list additions / version bumps). Dependabot-authored PRs are skipped, since they are already covered by the workflow above. - Both `verify` jobs start on every PR, because `verify` is a required status check on `main` (`.asf.yaml`) and a check that never starts would block a PR forever. Their first step asks the API for the PR's changed files; a PR that touches none of the three allow-list files (`actions.yml`, `approved_patterns.yml`, the Dependabot composite action) passes at once, with no checkout or build. Both jobs watch all three, so whichever runs for the PR's author covers every allow-list file. The decision is made inside the job, so a failed lookup fails `verify` rather than skipping it, and a file list the API may have cut short is verified rather than skipped. + Both `verify` jobs start on every PR, because `verify` is required on `main` by the `Required checks` ruleset in `.asf.yaml`, and a check that never starts would block a PR forever. Their first step asks the API for the PR's changed files; a PR that touches none of the three allow-list files (`actions.yml`, `approved_patterns.yml`, the Dependabot composite action) passes at once, with no checkout or build. Both jobs watch all three, so whichever runs for the PR's author covers every allow-list file. The decision is made inside the job, so a failed lookup fails `verify` rather than skipping it, and a file list the API may have cut short is verified rather than skipped. - **`check_action_tags` job in `check_action_tags.yml`** — triggers when `actions.yml`, `approved_patterns.yml`, the generated Dependabot composite action, the update workflow, or gateway verification code changes. It verifies that configured action SHAs exist and, when a `tag` is recorded, that the SHA is reachable from that Git tag or branch. -- **`approval` job in `allowlist_approval.yml`** — runs on every PR, on every review submitted, edited or dismissed, and on label changes. If the PR touches `actions.yml`, `approved_patterns.yml` or the generated Dependabot composite action, it fails until at least one maintainer (a reviewer with write access to this repository) has approved; only each reviewer's latest approve / request-changes / dismissal counts. A maintainer can waive the approval, for example on an allow-list PR they authored and so cannot approve, by adding the `allowlist-approval-override` label; it counts only while on the PR and only if whoever last added it has write access. Other PRs pass immediately. The check is a required status check on `main` (`.asf.yaml`), so an allow-list PR cannot be merged without one or the other. The logic lives in `scripts/check_allowlist_approval.py`, which the job runs from the PR's base commit so a PR cannot rewrite its own gate. +- **`approval` job in `allowlist_approval.yml`** — runs on every PR and on every review submitted, edited or dismissed. If the PR touches `actions.yml`, `approved_patterns.yml` or the generated Dependabot composite action, it fails until at least one maintainer (a reviewer with write access to this repository) has approved; only each reviewer's latest approve / request-changes / dismissal counts. Other PRs pass immediately. The check is required on `main` by the `Required checks` ruleset in `.asf.yaml`, so an allow-list PR cannot be merged unapproved. People with write access can deliberately merge past it ("bypass rules" on the PR), but cannot push past it; repository admins bypass it outright, which is how `update.yml`'s sync commit, pushed as the admin `asfgit`, still lands. The logic lives in `scripts/check_allowlist_approval.py`, which the job runs from the PR's base commit so a PR cannot rewrite its own gate. These workflows use regular `pull_request` (and `pull_request_review`) triggers with read-only permissions and no PR comments — pass/fail is surfaced through the status check. They do not auto-approve or merge; a human reviewer must still approve. diff --git a/scripts/check_allowlist_approval.py b/scripts/check_allowlist_approval.py index 99e32483..0e116452 100644 --- a/scripts/check_allowlist_approval.py +++ b/scripts/check_allowlist_approval.py @@ -32,11 +32,6 @@ repository. Only each reviewer's latest decisive review counts, so an approval later followed by "changes requested" or a dismissal does not. -A maintainer can waive the approval by adding ``OVERRIDE_LABEL`` -- for an -allow-list PR they authored themselves, which GitHub does not let them -approve. The label counts only while it is on the PR and whoever last added -it has write access: triagers can add labels too. - The decision logic is kept in pure functions for unit testing; ``main`` fetches the PR files, reviews and reviewer permissions through ``gh api``. @@ -64,8 +59,6 @@ # ``maintain`` is reported as ``write`` there. MAINTAINER_PERMISSIONS = frozenset({"admin", "maintain", "write"}) -OVERRIDE_LABEL = "allowlist-approval-override" - # Review states that set a reviewer's verdict. COMMENTED and PENDING do not # change it, matching how GitHub itself counts approvals. DECISIVE_STATES = frozenset({"APPROVED", "CHANGES_REQUESTED", "DISMISSED"}) @@ -97,29 +90,6 @@ def maintainer_approvers( ] -def override_by( - labels: Iterable[str], - label_events: Iterable[dict], - permission_of: Callable[[str], str], -) -> str | None: - """The maintainer whose override label waives the approval, or None. - - ``label_events`` must be in chronological order, as the API returns them. - A label with no visible ``labeled`` event counts for nothing: who added it - is unknown, and unknown must not be more permissive than a refusal. - """ - if OVERRIDE_LABEL not in labels: - return None - added = [ - e["login"] - for e in label_events - if e["event"] == "labeled" and e["label"] == OVERRIDE_LABEL - ] - if not added: - return None - return added[-1] if permission_of(added[-1]) in MAINTAINER_PERMISSIONS else None - - def _gh_lines(*args: str) -> list[str]: out = subprocess.run( ["gh", "api", *args], check=True, capture_output=True, text=True @@ -178,26 +148,9 @@ def main(argv: list[str] | None = None) -> int: if approved_by: print(f"Allow-list PR approved by maintainer(s): {', '.join(approved_by)}") return 0 - - labels = _gh_lines(f"repos/{args.repo}/issues/{args.pr}", "--jq", ".labels[].name") - label_events = [ - json.loads(line) - for line in _gh_lines( - f"repos/{args.repo}/issues/{args.pr}/events", - "--paginate", - "--jq", - '.[] | select(.event == "labeled" or .event == "unlabeled")' - " | {event, label: .label.name, login: .actor.login}", - ) - ] - waived_by = override_by(labels, label_events, lambda login: _permission_of(args.repo, login)) - if waived_by: - print(f"Maintainer approval waived by {waived_by} with the {OVERRIDE_LABEL} label.") - return 0 print( "::error::Allow-list PR has no approval from a maintainer " - "(a reviewer with write access to the repository). A maintainer can " - f"waive it with the {OVERRIDE_LABEL} label." + "(a reviewer with write access to the repository)." ) return 1 diff --git a/scripts/test_check_allowlist_approval.py b/scripts/test_check_allowlist_approval.py index 6008ba1a..958fe847 100644 --- a/scripts/test_check_allowlist_approval.py +++ b/scripts/test_check_allowlist_approval.py @@ -79,39 +79,3 @@ def test_no_maintainer_approval(): reviews = [_review("carol", "APPROVED"), _review("alice", "COMMENTED")] permissions = {"alice": "admin", "carol": "read"} assert caa.maintainer_approvers(reviews, permissions.__getitem__) == [] - - -def _labeled(login, label=None, event="labeled"): - return {"event": event, "label": label or caa.OVERRIDE_LABEL, "login": login} - - -def _permission(**perms): - return lambda login: perms.get(login, "read") - - -def test_override_by_a_maintainer_waives_approval(): - perms = _permission(alice="write") - assert caa.override_by([caa.OVERRIDE_LABEL], [_labeled("alice")], perms) == "alice" - - -def test_override_needs_the_label_on_the_pr(): - perms = _permission(alice="write") - events = [_labeled("alice"), _labeled("alice", event="unlabeled")] - assert caa.override_by([], events, perms) is None - - -def test_override_by_a_triager_does_not_count(): - perms = _permission(alice="write", tri="triage") - # A maintainer added it once, but whoever added it last is a triager. - events = [_labeled("alice"), _labeled("alice", event="unlabeled"), _labeled("tri")] - assert caa.override_by([caa.OVERRIDE_LABEL], events, perms) is None - - -def test_override_with_no_labeled_event_does_not_count(): - assert caa.override_by([caa.OVERRIDE_LABEL], [], _permission()) is None - - -def test_other_labels_are_not_an_override(): - perms = _permission(alice="write") - assert caa.override_by(["preview"], [_labeled("alice", label="preview")], perms) is None -