Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
0c880bfbb3 | ||
|
|
314aa77ee8 |
2 changed files with 68 additions and 0 deletions
|
|
@ -231,6 +231,9 @@ read them à la carte.
|
||||||
- **"How do I export Claude Code metrics (tokens, cost, tool calls) to
|
- **"How do I export Claude Code metrics (tokens, cost, tool calls) to
|
||||||
a Prometheus/Grafana collector? What OTEL options are available?"** →
|
a Prometheus/Grafana collector? What OTEL options are available?"** →
|
||||||
[`docs/observability.md`](docs/observability.md).
|
[`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
|
## Conventions & process
|
||||||
|
|
||||||
|
|
|
||||||
65
docs/pr-review-gate.md
Normal file
65
docs/pr-review-gate.md
Normal file
|
|
@ -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 <pr> --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.
|
||||||
Loading…
Reference in a new issue