diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a0ce849a..b1b23378 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -585,8 +585,67 @@ jobs: # on an empty list. Both of those are the same point, which is the point of the # whole workflow: a check nobody ran reads exactly like a check that passed, # and a gate that covers nothing is the purest form of that. + # The external reviewer is a check like any other, and it failed the way this + # gate's own comment warns about: silently. Sourcery answers every pull request + # with a *review*, and a review is what a reader looking for one finds -- so + # + # Sorry @blooop, you have reached your weekly rate limit of 500000 diff + # characters. + # + # arrives in the same shape as a review that found nothing, and reads as one to + # everything downstream. Twenty-six consecutive pull requests merged behind that + # sentence, among them the largest changes in the repo, and nothing said so. + # + # The classification is in `scripts/external_review_verdict.sh` rather than in + # this block, for the reason the public-api script is: a `case` statement that + # exists only inside a `run:` can be tested only by copying it, and the copy is + # what goes stale. What is here is the fetch, the polling and the label. + external-review: + runs-on: ubuntu-latest + permissions: + pull-requests: read + steps: + - uses: actions/checkout@v4 + - name: An external reviewer that refused is not an external reviewer + env: + GH_TOKEN: ${{ github.token }} + EVENT: ${{ github.event_name }} + PR: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + OVERRIDE_LABEL: no-external-review + run: | + set -euo pipefail + + if [ "$EVENT" != pull_request ]; then + echo "not a pull request, so there is nothing to have been reviewed" + exit 0 + fi + + if gh pr view "$PR" --repo "$REPO" --json labels --jq '.labels[].name' \ + | grep -qxF "$OVERRIDE_LABEL"; then + echo "::notice::'$OVERRIDE_LABEL' is set, so this pull request merges"\ + "without an external review. That is the label's whole purpose;"\ + "it is recorded here so the choice is visible afterwards." + exit 0 + fi + + # Sourcery posts within a minute of the pull request opening, well + # inside the shortest job in this workflow -- but "well inside" is not + # "before", and a race lost here fails a pull request for the + # reviewer's latency rather than for anything about the change. So + # poll, and let the remaining ~2 minutes of CI absorb the wait. + reviews= + for _ in $(seq 1 12); do + reviews=$(gh api "repos/$REPO/pulls/$PR/reviews" \ + --jq '.[] | select(.user.login == "sourcery-ai[bot]") | .body' || true) + [ -n "$reviews" ] && break + sleep 10 + done + + printf '%s' "$reviews" | ./scripts/external_review_verdict.sh + gate: - needs: [ci, e2e, rust, rust-coverage, public-api, packaging] + needs: [ci, e2e, rust, rust-coverage, public-api, packaging, external-review] if: always() runs-on: ubuntu-latest steps: diff --git a/CHANGELOG.md b/CHANGELOG.md index 223680ce..5cd435cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **CI fails when the external reviewer did not actually review.** Sourcery + answers a quota refusal *as a review*, so `Sorry @blooop, you have reached your + weekly rate limit…` arrives in the same shape as a review that found nothing. + Twenty-six consecutive pull requests merged behind that sentence — the largest + changes in this repo among them — and nothing anywhere said so. The new + `external-review` job, inside `gate`'s `needs`, refuses a quota refusal, a + refusal on diff size (which fires on exactly the changes least safe to merge + unread), and no review at all. The way past it is the `no-external-review` + label, so skipping external review is a decision on a named pull request rather + than a default that quietly stopped applying. + ## [0.13.0] - 2026-08-24 ### Changed diff --git a/README.md b/README.md index e2a7f648..96bccf76 100644 --- a/README.md +++ b/README.md @@ -2362,6 +2362,34 @@ means adding it to `gate`'s `needs`, in the same pull request, where it can be seen. It reaches only as far as its own workflow file, so the `prek` lint job is not behind it and has to be required alongside it. +One of `gate`'s jobs is not a test. `external-review` reads the pull request's +reviews and fails unless the external reviewer actually reviewed the code. It is +there because that reviewer failed silently once and nobody noticed for a day and +a half: Sourcery answers a quota refusal *as a review*, so + +``` +Sorry @blooop, you have reached your weekly rate limit of 500000 diff characters. +``` + +arrives in the same shape as a review that found nothing and reads as one to +everything downstream. Twenty-six consecutive pull requests merged behind that +sentence, among them the largest changes in the repo. The other refusal is worse +targeted still — a per-pull-request diff cap, which fires on exactly the changes +least safe to merge unread. + +So the job refuses three things: a refusal on quota, a refusal on size, and no +review at all after two minutes of polling. It is a failure rather than a warning +because a warning is precisely what already did not work, and the way past it is +the `no-external-review` label — a decision recorded on a named pull request, +rather than a default that quietly stops applying. Labelling a change is fine; +not knowing the reviewer stopped reading is not. + +The classification itself is `scripts/external_review_verdict.sh`, which the +workflow calls and `test/test_external_review_guard.py` executes, for the reason +the public-API script is a script: a `case` statement inside a `run:` block can +be tested only by copying it, and the copy is the half that goes stale. Its tests +run against the refusal bodies those twenty-six pull requests actually received. + Running it yourself is a different proposition. This repo's devcontainer carries a Docker daemon of its own, through the `docker-in-docker` feature, and pins the same devpod a host installs, so `pixi run test-e2e` from inside it builds its diff --git a/scripts/external_review_verdict.sh b/scripts/external_review_verdict.sh new file mode 100755 index 00000000..bf9eff1a --- /dev/null +++ b/scripts/external_review_verdict.sh @@ -0,0 +1,45 @@ +#!/usr/bin/env bash +# The verdict on whether an external reviewer actually reviewed a pull request. +# +# Separate from the workflow that calls it so that the classification is +# executable outside CI, which is what `test/test_external_review_guard.py` +# executes. A `case` statement living only inside a `run:` block is testable +# only by copying it, and a copied guard is a guard that drifts. +# +# Reads the concatenated review bodies on stdin. Exit 0 means an external +# reviewer answered about the code; exit 1 means it did not, and the reason is +# on stdout as a GitHub workflow-command error so the annotation appears on the +# pull request rather than only in a log nobody opens. +set -euo pipefail + +OVERRIDE=${OVERRIDE_LABEL:-no-external-review} +reviews=$(cat) + +if [ -z "${reviews//[[:space:]]/}" ]; then + echo "::error::no external review. Either the reviewer is not installed on"\ + "this repository any more, or it is not reaching this pull request."\ + "Label '$OVERRIDE' to merge anyway." + exit 1 +fi + +# Both refusals are quota refusals, and neither says anything about the code. +# They are matched on the stable half of each sentence: the numbers move when +# the plan does, and `@blooop` is not the only account that can open a pull +# request here. +case $reviews in + *"you have reached your weekly rate limit"*) + echo "::error::the external reviewer is out of weekly quota and reviewed"\ + "nothing. It posts that refusal *as a review*, which is why this is"\ + "not otherwise visible. Wait for the quota, or label '$OVERRIDE' to"\ + "merge unreviewed." + exit 1 + ;; + *"larger than the review limit"*) + echo "::error::this pull request is over the external reviewer's per-diff"\ + "cap, so it went unreviewed -- on the size of change that least"\ + "tolerates that. Split it, or label '$OVERRIDE' to merge unreviewed." + exit 1 + ;; +esac + +echo "the external reviewer ran and answered about the code" diff --git a/test/test_external_review_guard.py b/test/test_external_review_guard.py new file mode 100644 index 00000000..772d6cf1 --- /dev/null +++ b/test/test_external_review_guard.py @@ -0,0 +1,119 @@ +"""The guard that notices when the external reviewer stopped reviewing. + +Between 2026-08-22 and 2026-08-24, twenty-six consecutive pull requests merged +with no external review. Sourcery had run out of weekly quota and was answering + + Sorry @blooop, you have reached your weekly rate limit of 500000 diff + characters. + +-- posted *as a review*. So `gh pr view --json reviews` showed a review present, +the merge path was satisfied, and the outage was invisible for a day and a half. +The largest changes in the repo went through in that window, including a +breaking CLI grammar change and a 1,862-line feature that was withdrawn again +twenty-two hours later. + +The strings below are the real refusals, copied from those pull requests, and +the accepted body is the real "reviewed your changes and they look great" +Sourcery posts when it found nothing -- which is the case the guard must NOT +fail, and the case a naive "did the review say anything useful" check gets +wrong. + +`gate`'s `needs` is asserted here for the same reason `test_bench_workflow.py` +asserts the bench job is *absent* from it: the wiring is one edit away from +being lost, and a guard nothing requires is not a guard. +""" + +import subprocess +from pathlib import Path + +ROOT = Path(__file__).parent.parent +VERDICT = ROOT / "scripts" / "external_review_verdict.sh" +CI = ROOT / ".github" / "workflows" / "ci.yml" +JOB = "external-review" + +# Verbatim from PRs #343, #344, #345, #347, #348, #350-#368 and #370-#376. +WEEKLY_QUOTA = ( + "Sorry @blooop, you have reached your weekly rate limit of 500000 diff " + "characters.\n\nPlease try again later or [upgrade](https://app.sourcery.ai/" + "login?connection=github)" +) +# Verbatim from PR #369, the 1,631-line `--rm` grammar change. +OVER_DIFF_CAP = ( + "Sorry @blooop, your pull request is larger than the review limit of 150000 diff characters" +) +# Verbatim from PRs #330, #331, #333, #334, #337 and #342. +REVIEWED_AND_CLEAN = ( + "Hey - I've reviewed your changes and they look great!\n\n***\n\n" + "
\nSourcery is free for open source" +) +REVIEWED_WITH_FINDINGS = "Hey - I've found 2 issues\n\n## Individual Comments" + + +def verdict(bodies: str) -> subprocess.CompletedProcess: + return subprocess.run( + ["bash", str(VERDICT)], + input=bodies, + capture_output=True, + text=True, + check=False, + ) + + +def test_the_weekly_quota_refusal_is_not_a_review(): + result = verdict(WEEKLY_QUOTA) + assert result.returncode == 1, "the outage that started this ran for 26 PRs" + assert "out of weekly quota" in result.stdout + + +def test_a_pull_request_over_the_diff_cap_is_not_reviewed(): + # The worse of the two: it fires on size, so it exempts exactly the changes + # that least tolerate going unread. #369 was 1,631 lines of CLI grammar. + result = verdict(OVER_DIFF_CAP) + assert result.returncode == 1 + assert "per-diff cap" in result.stdout + + +def test_no_review_at_all_is_not_a_review(): + for empty in ["", "\n", " \n "]: + result = verdict(empty) + assert result.returncode == 1, f"{empty!r} left the reviewer unaccounted for" + assert "no external review" in result.stdout + + +def test_a_review_that_found_nothing_passes(): + # The case that makes this guard usable rather than merely loud: most + # reviews find nothing, and failing those would get the job deleted. + result = verdict(REVIEWED_AND_CLEAN) + assert result.returncode == 0, result.stdout + + +def test_a_review_that_found_something_passes(): + result = verdict(REVIEWED_WITH_FINDINGS) + assert result.returncode == 0, result.stdout + + +def test_a_refusal_among_several_reviews_still_fails(): + # Bodies arrive concatenated. A re-review after a push can leave a real + # review and a later refusal side by side, and the refusal is the newer + # fact: the current head is what went unread. + result = verdict(REVIEWED_AND_CLEAN + "\n" + WEEKLY_QUOTA) + assert result.returncode == 1 + + +def test_the_gate_requires_the_job(): + ci = CI.read_text(encoding="utf-8") + assert f"\n {JOB}:\n" in ci, f"{JOB} is the job this file is about" + needs = next(line for line in ci.splitlines() if line.strip().startswith("needs: [ci,")) + assert JOB in needs, ( + "a job outside `gate`'s needs is not required by the branch ruleset, " + "which is the same shape of nothing-gating-anything this guard exists " + "to catch" + ) + + +def test_the_workflow_runs_this_script_rather_than_its_own_copy(): + ci = CI.read_text(encoding="utf-8") + assert "./scripts/external_review_verdict.sh" in ci, ( + "the classification must be the one under test; a copy inside a `run:` " + "block is what goes stale" + )