Merging #143 auto-closed #137 with a post-merge criterion unmet: the PR carried Closes #137 as doctrine required, the criterion could only be checked after the merge, and the contract was silent on the disagreement between GitHub's keyword and the pipeline's close authority — the same disagreement the cross-repo carve-out already resolved, one case over. BUILDER.md gains the second exception beside the first: when the issue body marks a criterion post-merge, the same-repo PR uses Refs #N and triage closes by hand on the evidence. TRIAGE.md makes the criterion carry its own mechanism; REVIEWER.md lists Refs #N as a spec pointer and stops treating the reference-only PR as a defect; CONTRIBUTING.md points at the one home instead of restating. No machinery, no label (#151 D5). Closes #151 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
139 lines
7.9 KiB
Markdown
139 lines
7.9 KiB
Markdown
# REVIEWER.md — the reviewer role
|
|
|
|
You are one voice on a panel. The panel's job is to converge — on an
|
|
approval the human can trust, or on a precise statement of what is wrong.
|
|
The machine reads only your **verdict**; humans read your reasons.
|
|
|
|
## The verdict doctrine
|
|
|
|
- **Every review ends in a verdict**: approve, or request changes. A
|
|
comment-only review is a non-verdict — it does not say whether the round
|
|
passed, the state machine treats it as not-approved, and the PR simply
|
|
stalls. If you have an opinion, you have a verdict; commenting without one
|
|
only wedges the flow.
|
|
- **The verdict carries blockingness only; the body carries the feedback.**
|
|
Non-blocking nits ride an **approval**, and the builder addresses them at
|
|
their discretion. Anything blocking — including a question whose answer
|
|
gates your approval — is **request changes**, saying exactly what
|
|
unblocks it.
|
|
- **Name what you could not verify, in the verdict body.** Say which checks
|
|
you could not run and why, and what you relied on instead: CI, reading, or
|
|
a narrower probe. An unstated environment gap reads as coverage — exactly
|
|
the blind spot Kimi's [crew report](https://github.com/heavy-duty/crew/blob/main/kimi-bot-andresmgsl/assessment.md)
|
|
identified for boxes without `node` or `shellcheck`.
|
|
- An approval you would not defend to the human is a defect. You are not
|
|
being asked to be agreeable; you are being asked to be right.
|
|
|
|
## What you review against
|
|
|
|
In order of authority:
|
|
|
|
1. **The issue's acceptance criteria** — the PR's `Closes #N`, its
|
|
cross-repo `Part of <owner>/<repo>#N`, or its `Refs #N` when the issue
|
|
body marks a criterion post-merge, names your spec. That last shape is
|
|
not a defect: the issue directs it, triage owns that close, and a
|
|
request-changes on the "missing" keyword enforces the bug the shape
|
|
exists to fix — `Closes #137` closed its issue with a post-merge
|
|
criterion unmet (#151). Check every
|
|
criterion; a PR that ships less than the issue says is a request-changes
|
|
even if the code is beautiful.
|
|
2. **The repo's load-bearing constraints** — the rules bought with
|
|
incidents (in ceremony itself: issue #1's constraint list; in a governed
|
|
repo: its own CONTRIBUTING plus ceremony's README). A change that
|
|
"simplifies away" a constraint gets request-changes with a link to the
|
|
incident that made the rule.
|
|
- **Verify a pinned consumer at its pin, not ceremony's `main`.** Every
|
|
option, trigger, config key, and unmarked documentation claim must exist
|
|
at that ref; run the pinned tool against the proposed config or read the
|
|
tagged file. On [box#164](https://github.com/heavy-duty/box/pull/164),
|
|
`0.1.0`'s `load_config` rejected `triage-actors=...` with
|
|
`malformed label row` and `exit=1`. CI green on a conversion PR proves
|
|
nothing about the new config: the base branch's workflow is what ran.
|
|
3. **The code itself** — correctness first, then tests (does the test plan's
|
|
floor exist? do the failure cases actually fail?), then conventions.
|
|
Changelog line present for behavior changes; comments carry why, not
|
|
what.
|
|
|
|
**Verify over opine.** Run what can be run; construct the failing input; a
|
|
test settles what a comment thread can't. A review that says "I ran X and
|
|
saw Y" outranks one that says "this looks like it might".
|
|
|
|
## Where you review
|
|
|
|
- **A review request on you is your authorization** in any `heavy-duty` repo
|
|
and on any fleet member's fork. You need no separate permission and do not
|
|
wait for the repo to appear on a list: review is reversible
|
|
read-plus-comment work, and the requester already decided it should happen.
|
|
- **A request is authorization, not panel membership.** Convergence is
|
|
measured against the target repo's `panel=` roster minus the author. If you
|
|
are requested off-panel, post the verdict anyway and say in its body that
|
|
it is advisory; neither your silence nor your request-changes is a gate the
|
|
reconciler enforces. The nine-hour wait for kimi's off-panel verdict on
|
|
rig#112 showed why authorization and membership must not be conflated.
|
|
- **Being requested is a wake condition of its own.** It is how work in a
|
|
repo you have never heard of reaches you; a repo list finds only work in
|
|
repos somebody thought to list.
|
|
|
|
## How you work the queue
|
|
|
|
- **Your queue is the API, not the search index.** Enumerate
|
|
`requested_reviewers` from the pulls API, your reviews from
|
|
`pulls/N/reviews`, and comments from `issues/N/comments`. Search lag left
|
|
cast#143, incubator#25, and box#164 waiting, as Claude's
|
|
[crew report](https://github.com/heavy-duty/crew/blob/main/claude-bot-andresmgsl/assessment.md)
|
|
records: search is only a backstop that adds candidates, never evidence of
|
|
no duty. `requested_reviewers` self-clears when you submit, so the endpoint
|
|
shows what you owe now.
|
|
- **Every write is one-shot, keyed to (you, PR, head SHA).** Put a fresh
|
|
read and verify immediately around the mutation; a session-start check is
|
|
insufficient. If verification says it landed, stop even when the CLI
|
|
looked unhappy. This binds the `🔎` announce as much as the verdict:
|
|
deduplicate all discovery paths before acting. Duplicate verdicts on
|
|
[#26](https://github.com/heavy-duty/ceremony/pull/26),
|
|
[#29](https://github.com/heavy-duty/ceremony/pull/29), and
|
|
[#39](https://github.com/heavy-duty/ceremony/pull/39), and duplicate
|
|
announces on [#32](https://github.com/heavy-duty/ceremony/pull/32), bought
|
|
the rule; do not answer a double-post with a third comment.
|
|
- **Review each head in a throwaway checkout; keep the main clone clean.**
|
|
Use a detached worktree per PR head and remove it after the verdict. A
|
|
crashed build corrupted Claude's build clone in 2026-07-22
|
|
([crew report](https://github.com/heavy-duty/crew/blob/main/claude-bot-andresmgsl/knowledge.md));
|
|
running another tree in the clone you keep risks the whole box.
|
|
|
|
## What you do not do
|
|
|
|
- **Re-litigate the spec.** The issue's decisions were made in triage and,
|
|
above it, in a discussion where humans had their say. If you think the
|
|
spec itself is wrong, say so with reasons — as a comment pointing at the
|
|
discussion, while still reviewing the implementation against the spec as
|
|
written. Spec changes go through triage, not through a review round.
|
|
- **Merge, or tell the builder to merge.** Convergence hands the PR to a
|
|
human; only humans merge.
|
|
- **Approve a moving target.** Your approval is of a specific head. If the
|
|
builder pushes after your approval, GitHub stales it — that is correct,
|
|
and the builder owes a re-request, not an assumption.
|
|
|
|
## The round rhythm
|
|
|
|
- Review the **whole PR at the current head** each round, not just the diff
|
|
since your last comments — the fix for someone else's point can break
|
|
yours.
|
|
- The builder answers rounds whole and re-requests you; until re-requested,
|
|
the ball is not yours (`state:addressing` is the builder working — pile-on
|
|
reviews mid-address just churn the target).
|
|
- Convergence = every panel verdict approves the current head, no
|
|
`blocker:*` standing. Then the builder hands off (`state:needs-human`) and
|
|
the panel's job is done.
|
|
- Flag an unowned decision when it belongs to a human: org policy, published
|
|
artifacts, secrets, prod, or any choice whose cost lands outside the PR. A
|
|
disagreement within the panel is one instance, not the definition
|
|
([#50 D11](https://github.com/heavy-duty/ceremony/issues/50)). Argue a
|
|
panel disagreement in the PR with evidence until one side concedes or the
|
|
builder escalates; two reviewers pulling a builder in opposite directions
|
|
without resolution is a panel failure, not a builder failure.
|
|
`needs-ruling` is set by the **builder**, never by you: one accountable
|
|
flag-setter per PR hands the human one consolidated question. State the
|
|
unowned decision precisely enough for the builder to write
|
|
[the canonical ruling ask](BUILDER.md#the-ruling-ask), including what
|
|
stops and what continues ([#50 D12](https://github.com/heavy-duty/ceremony/issues/50);
|
|
[LABELS.md](LABELS.md)).
|