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..f4ae3fad --- /dev/null +++ b/docs/pr-review-gate.md @@ -0,0 +1,65 @@ +# The PR review gate + +What a review verdict means, why a reviewer shouldn't wait on CI to +submit one, and what "armed to auto-merge" actually signals about the +human review that already happened. + +## The gate has (up to) three parts, and they're per-repo settings + +Whether a PR can merge, and what counts toward "can", is configured +per repo in its branch-protection settings — not a fact true of every +hive or every repo. The pieces a repo *can* require: + +- **CI is green** — the repo's required status checks pass on the + PR's current head commit, if the repo requires any. +- **Requested reviews are `APPROVED`** — and whether a review is + invalidated by a later commit ("stale") is itself a setting + (Forgejo's "dismiss stale approvals" branch-protection option), not + universal behavior. +- **Someone with write access has armed the PR to merge** — a manual + merge once the required conditions hold, or Forgejo's auto-merge + (merges automatically the moment the other required conditions are + met). + +Where these are required, they're independent of each other. A +reviewer only ever owns the review-approval piece — CI resolves (or +doesn't) on its own regardless of what a review says, and merge-arming +is someone else's call. + +## Reviewers: submit the verdict, don't gate it on CI + +Submit `hive-forge pr-reviews --approve` or `--request-changes` +as soon as you've finished checking the diff — don't hold it back +waiting for CI to go green first. 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 gate the formal verdict on it: CI isn't a +signal a reviewer waits on, it's a separate condition that resolves +independently. + +## What arming auto-merge actually means + +Auto-merge isn't "no human ever looked at this." Whoever arms it has +already judged the PR sound at a coarse level — the signal it sends is +roughly *"apart from maybe minor tweaks a reviewer can still catch, +I think this is fine."* That's the human-in-the-loop step, and it +already happened. No large changes are expected to surface after +that point — a reviewer's job past that point is to flag it if one +does, not to assume none ever will. + +The practical consequence for a reviewer: on a repo where auto-merge +may already be armed before your review lands, a plain `APPROVED` can +be the last step before the merge actually happens, with no further +review pass after yours. That's a reason to actually finish checking +before approving — not a reason to hesitate over every small thing. +Genuine, substantive doubt (a claim you haven't verified, a real +correctness question) is worth a `request-changes` or a clarifying +comment before approving; a stray style nit isn't the same category. + +## Re-review after a repo requires it + +Where a repo dismisses stale approvals on a new commit, a review you +already gave stops counting the moment a follow-up commit lands — even +one whose message reads as trivial ("just a wording fix", "just +trimming comments"). Re-diff and re-verify before submitting a fresh +verdict; don't take a small-sounding commit message as an accurate +description of the diff.