diff --git a/CLAUDE.md b/CLAUDE.md index 8d01ce66..584e21b5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -231,6 +231,9 @@ read them à la carte. - **"How do I export Claude Code metrics (tokens, cost, tool calls) to a Prometheus/Grafana collector? What OTEL options are available?"** → [`docs/observability.md`](docs/observability.md). +- **"What does a PR review verdict actually gate? Should a reviewer + wait for CI before approving? What does arming auto-merge commit an + operator to?"** → [`docs/pr-review-gate.md`](docs/pr-review-gate.md). ## Conventions & process diff --git a/docs/pr-review-gate.md b/docs/pr-review-gate.md new file mode 100644 index 00000000..46cb4105 --- /dev/null +++ b/docs/pr-review-gate.md @@ -0,0 +1,84 @@ +# The PR review gate + +What a review verdict means in this hive, why a reviewer shouldn't +wait on CI to submit one, and what an operator setting up a fresh +hive should know before arming auto-merge. Prompted by a live +mistake: a reviewer held a formal approve/request-changes verdict +back "until CI is green," which is not the reviewer's job to gate on +and just delayed a merge that was already waiting on the review alone +(hyperhive/hyperhive#3050). + +## The gate has three independent parts + +A PR merges when **all** of: + +1. **CI is green** — the repo's required status checks pass on the + PR's current head commit. +2. **Every requested review is `APPROVED`, and none is stale** — + Forgejo marks a review stale the moment a new commit lands on the + branch after it; a stale approve does not count. +3. **Someone with write access has armed the PR to merge** — either a + manual merge once 1 and 2 hold, or Forgejo's auto-merge (merge the + instant the other two conditions are met, no human back in the + loop at that moment). + +These three are independent. A reviewer is only ever responsible for +part 2. Waiting on part 1 before submitting a verdict doesn't make +the review "more correct" — it just delays the gate for no reason, +since CI resolves on its own regardless of what the review says. + +## Reviewers: submit the verdict, don't gate on CI + +Submit `hive-forge pr-reviews --approve` or `--request-changes` +as soon as you've finished checking the diff — **not** once CI +happens to be green. Mention CI's current state in the review body if +it's relevant (e.g. "approving; `nix flake check` is still running"), +but don't hold the formal verdict hostage to it. CI is not a signal a +reviewer waits on; it's a parallel, independent gate that resolves on +its own. + +This matters concretely because of how auto-merge interacts with +review timing: + +**An operator may arm auto-merge before your review lands.** If they +have, and CI is already green, your `APPROVED` review is the +*trigger* — the merge happens the instant Forgejo records your +review, with no further human check. There is no "let me look this +over once more before it actually merges" step once you've approved. + +The practical consequence: **lean hard toward `request-changes` when +in doubt**, including for things that feel minor — a comment that's +slightly wrong, a claim you haven't fully verified, a doc that might +be stale. On a repo without auto-merge armed, an unnecessary +`request-changes` costs a re-review round-trip. On one where +auto-merge is armed, an `approve` you weren't fully sure of ships +immediately. That asymmetry is why the default under uncertainty is +to hold, not wave through. + +Corollary: don't read "submit promptly" as "submit hastily." The +fix isn't rushing the check, it's not padding the check with an +extra, unrelated wait (CI) that was never yours to hold. + +## Re-review after every push + +A stale review (Forgejo flags it in the PR's review list once a new +commit lands) does not count toward the gate. If a PR you've reviewed +gets a follow-up commit — even a "just a wording fix" or "just +trimming comments" commit — re-diff it and submit a fresh verdict +before it can merge. Don't assume a small-sounding commit message +accurately describes the diff; verify it the same way you verified +the original. + +## Operators: what arming auto-merge actually commits you to + +Setting a PR to auto-merge in Forgejo means: the moment CI is green +**and** every requested review is an approve, the PR merges — with no +further chance for a human to look at it. If you arm it *before* a +review has landed, you are trusting that reviewer's eventual +`APPROVED` completely; there's no second gate after them. + +If that's not what you want for a given PR (e.g. you want to glance +at it yourself even after the requested reviewer approves), don't arm +auto-merge — merge manually once the gate conditions are met, or add +yourself as a required reviewer too so the gate's part 2 doesn't +close on the requested reviewer's approve alone.