diff --git a/.github/workflow-templates/two-human-reviewers.caller.yml b/.github/workflow-templates/two-human-reviewers.caller.yml index 8960faf..7560ea6 100644 --- a/.github/workflow-templates/two-human-reviewers.caller.yml +++ b/.github/workflow-templates/two-human-reviewers.caller.yml @@ -9,29 +9,11 @@ name: Two human reviewers # directly. # # END template-only -# The logic lives in c-build-tools so every repository shares one definition; see -# `.github/workflows/two-human-reviewers.yml` there for why this is a workflow rather than a branch rule, -# and for the `required-approvals` and `exempt-bots` inputs. -# -# Keep BOTH triggers below. A called workflow's own triggers are ignored, so these are what re-run the -# gate; drop `pull_request_review` and it evaluates once at open, finds no approvals, and never re-runs -# when someone approves - leaving the required check failing until an unrelated push happens to re-trigger -# it. -# -# `pull_request_target` rather than `pull_request`, because the gate publishes its verdict as a commit -# status and a pull request from a fork carries a read-only token on `pull_request`. It could never -# publish for one, and the fork's pull request would then wait forever on a status that never arrives. -# `pull_request_target` runs in the base repository's context, with a token that can write. -# -# That is only safe because the gate never checks out the pull request's code - it reads the API and -# nothing else. Do not add a checkout here, and do not add one to the called workflow: with -# `pull_request_target` that code would run with a writable token against the base repository. -# -# Do not add a required status check for this until the file is on the default branch, or every open pull -# request will wait forever on a check that cannot run. Pull requests that were already open when it landed -# get no event either, and are unstuck by reopening them, or by any push to them. Dispatching does not do -# it: a dispatch runs against the default branch, so its check run attaches to that branch's commit rather -# than to a pull request's head. It reports the verdict to the run log, and is useful for that. +# Calls the shared gate in c-build-tools. Keep `pull_request_review`, or approvals never re-run it. A +# fork's PR gets a read-only token (`pull_request_target` is blocked by default), so a maintainer +# publishes its status with "Run workflow" or the command below, again after each push or review. +# Never add a checkout. +# gh workflow run two-human-reviewers.yml -R / -f pr= on: workflow_dispatch: @@ -40,7 +22,7 @@ on: description: PR number to evaluate. type: string required: true - pull_request_target: + pull_request: types: [opened, reopened, synchronize, ready_for_review] pull_request_review: types: [submitted, dismissed] diff --git a/.github/workflows/two-human-reviewers.yml b/.github/workflows/two-human-reviewers.yml index 0124c28..2842fdf 100644 --- a/.github/workflows/two-human-reviewers.yml +++ b/.github/workflows/two-human-reviewers.yml @@ -1,61 +1,8 @@ name: Two human reviewers -# Bot-authored pull requests need approvals from two distinct humans (see `required-approvals`). -# -# A human-authored PR already has two people in the loop: the author who wrote and vetted the change, -# and the approver. When the agent opens the PR that first human is missing, so a single approval would -# be the only human judgement applied to it. This restores the second. -# -# This is the shared definition. Other repositories call it from a thin caller workflow, so the logic -# is maintained once; it also triggers directly on this repository's own PRs, which is why it declares -# `pull_request` alongside `workflow_call`. When called, `github.repository` and the event payload are -# the caller's, so it gates the calling repository's PR. -# -# A called workflow's own triggers are ignored - only the caller's events run it - so a caller MUST -# declare `pull_request_review` as well as `pull_request_target`. Without it the gate evaluates once at -# open, finds no approvals, and never re-runs when humans approve, leaving the required check stuck. -# `.github/workflow-templates/two-human-reviewers.caller.yml` is the template to copy. -# -# `pull_request_target` rather than `pull_request`, because the verdict is published as a commit status -# and a pull request from a fork carries a read-only token on `pull_request`; it could never publish for -# one, and that pull request would wait forever on a status that never arrives. `pull_request_target` -# runs in the base repository's context with a token that can write. -# -# It never checks out the pull request's code - it reads the API and nothing else - which is what makes -# `pull_request_target` safe here, and what makes running on `pull_request_review` safe too, since both -# carry a writable token. Do not add a checkout to this workflow. -# -# Because both triggers take the definition from the base branch rather than from the pull request, a -# pull request can no longer be judged by its own edited copy of this file. Deleting or weakening it now -# takes effect only once merged. It is still security-relevant: review a diff that touches it as such, -# and requiring code owner review on `.github/workflows/**` in branch protection is what enforces that. -# -# Why a job and not a branch rule: GitHub cannot condition an approval count on the PR author, and a -# required check that never reports blocks a PR forever ("Expected - waiting for status to be -# reported"). So this runs on every PR and passes immediately for human authors, and only bot-authored -# PRs are actually gated. -# -# The unit being judged is the head commit, not the pull request that triggered the run. A check run -# attaches to a commit, and branch protection reads the newest run of a given name on the pull request's -# head commit - so two pull requests that share a head commit share this check, and whichever run -# finishes last decides both. Copying a bot's branch and opening a second pull request from it is enough -# to produce that, and judging only the triggering pull request would then let the copy's "author is -# human, not gated" verdict land on the bot's pull request and clear it. Every open pull request on the -# commit is therefore evaluated together, which makes the verdict a function of the commit alone and -# leaves the order the runs finish in unable to change it. The cost is that a human's pull request -# sharing a commit with a bot's is held to the bot's requirement; that is the safe direction, and the -# approvals it collects count toward the shared commit. -# -# Gating on "the author is a bot" rather than on a list of named bots is deliberate: an app can be -# renamed, and a stale name would silently let its pull requests through under-reviewed. Exempting a bot -# is therefore an explicit act, and forgetting to update this file can only ever be too strict. -# -# `dependabot[bot]` is exempt. The gate exists because an agent writes the change a human would -# otherwise have written and vetted, leaving one human short. A dependency bump has no missing author: -# the change is a version number, and the code it pulls in was never going to be written here. The -# repository's normal review rule is the control that fits it. -# -# It deliberately never checks out the PR's code, so running on `pull_request_review` (which carries a -# writable token) cannot execute anything from the branch under review. +# Bot-authored PRs need approvals on the head commit from two distinct humans with push access; the +# `mrbot/two-human-reviewers` status reports the verdict. Fork PRs get a read-only token, so they need a +# manual run (`pull_request_target` is blocked by default). Never check out PR code, and require code +# owner review on `.github/workflows/**`. on: workflow_call: @@ -76,18 +23,17 @@ on: to it. type: string required: false - # Dispatching runs against the default branch, so the check run it produces attaches to that branch's - # commit rather than to a pull request's head, and no pull request is unblocked by it. It reports the - # verdict to the run log, which makes it useful for checking this logic against a real pull request. A - # pull request that was already open when the gate landed has no event to evaluate it with - those don't - # fire retroactively - and is unstuck by reopening it, or by any push to it. + # A manual run uses the default branch's copy with a token that can write, and publishes the status to + # the PR's head commit. Start it with "Run workflow" or the command below; re-running a fork PR's own + # run keeps its read-only token. Repeat it after each push or review. + # gh workflow run two-human-reviewers.yml -R / -f pr= workflow_dispatch: inputs: pr: description: PR number to evaluate. type: string required: true - pull_request_target: + pull_request: types: [opened, reopened, synchronize, ready_for_review] pull_request_review: types: [submitted, dismissed] @@ -137,14 +83,13 @@ jobs: # # Publishing is best effort. It needs `statuses: write`, which a called workflow only has if its # caller granted it, and a `pull_request` run on a fork's pull request holds a read-only token - # whatever the caller granted. Neither case may fail the run: the job's own exit code is what - # those repositories still gate on, and turning a missing permission into a red check would - # block them on the automation rather than on the review. + # whatever the caller granted. Neither case fails the run: branch protection reads the status, + # which stays unchanged until a run that can write publishes it. publish() { if ! gh api --method POST "repos/$REPO/statuses/$head_sha" \ -f state="$1" -f context="$STATUS_CONTEXT" -f description="$2" \ -f target_url="$RUN_URL" >/dev/null 2>&1; then - echo "::warning::Could not publish the $STATUS_CONTEXT status. The run needs 'statuses: write', and a fork's pull request cannot be published to from a pull_request trigger." + echo "::warning::Could not publish the $STATUS_CONTEXT status: the run needs 'statuses: write', which pull_request and pull_request_review runs on a fork's pull request never get. A maintainer can publish it with Run workflow (not Re-run) and pr=$PR, after each push or review." fi }