atelier/review/peer-review-checklist.md

Peer Review Checklist

For human reviewers. Run this when reviewing a PR or a change. Complements agent-checklist.md (which the author ran before finishing).

Purpose

A peer review is a second set of eyes on correctness, clarity, and completeness. It is not a gatekeeping ritual; it is a quality multiplier. The author ran the agent checklist; the reviewer checks what the author could not see.

The Review

Understanding (read first, judge never)

• Read the PR description. What is the change trying to do?
• Read the linked issue/spec. Does the change address it?
• Read the changed files in order. Do not jump to judgment.

Correctness (C1)

• Does the change do what it claims?
• Are there edge cases the author missed? (Comment, don't fix in review)
• Are there failure cases unhandled?
• Are the tests testing the right thing? (A test that cannot fail is theater)
• Would this code fail in production? (Different from "does it pass CI")

Clarity (C2)

• Can you understand the change without asking the author?
• Are names intent-revealing?
• Is there a comment that explains why where the why is non-obvious?
• Is the diff minimal? (Unrelated changes are review noise)

Simplicity (C3)

• Is there dead code introduced?
• Is there premature abstraction?
• Could the change be simpler? (Suggest, don't block unless egregious)
• Is there a simpler approach the author may not have considered?

Locality (C4)

• Are related changes grouped?
• Are unrelated changes separated (different PRs)?
• Does a change require touching distant files unnecessarily?

Reversibility (C5)

• Is the change undoable?
• Are there migrations? Do they have rollbacks?
• Are there breaking changes? If so, is there a deprecation path?

Composability (C6)

• Does the change respect existing boundaries?
• Does it introduce coupling that should be an interface?
• Is the new code reusable, or one-off?

Observability (C7)

• Are there logs/metrics for the new behavior?
• Are errors structured and traceable?
• Are there no secrets in logs?

Economy (C8)

• Are there unbounded operations?
• Is memory/time bounded?
• Is the cost proportional to the need?

Domain-Specific (if applicable)

Run the relevant domain section from agent-checklist.md (UI/UX, API, Security, Data, Testing, Performance, Observability, Errors, Concurrency, DevOps, Infrastructure as Code, Kubernetes). The author ran it; the reviewer verifies.

If Infrastructure as Code

• Are providers pinned and modules version-pinned? (no latest)
• Is state remote with locking, not committed?
• Is the plan diff reviewed, not just the config?
• Are secrets via providers/stores, not in HCL?
• Are credentials scoped per environment?
• Is drift being treated as an incident?

If Kubernetes

• Are controllers used, not bare pods?
• Are resource requests set on every prod container?
• Are probes defined and checking the workload, not its dependencies?
• Is RBAC least-privilege, bound to ServiceAccounts by intent?
• Are image tags pinned, not :latest?
• Is persistent storage explicit (PVC + reclaim policy decided)?
• Are ConfigMaps and Secrets separate?
• Is the rollback path tested, not assumed?

Review Etiquette

• Comment, don't command. "This could be X" not "Change this to X."
• Distinguish blocking from suggestions. "Blocking: correctness bug. Suggestion: naming."
• Praise good code. Reviews are not just for finding problems.
• Don't review style the linter should catch. Fix the linter, not the PR.
• Ask questions. "Why this approach?" often reveals more than "This is wrong."

Approving

• Approve when: the change is correct, clear, and complete. Minor suggestions can be left for the author.
• Request changes when: there is a correctness bug, a missing test, or a clarity problem that blocks understanding.
• Reject when: the change should not exist (wrong direction, duplicate, scope-violating).

What This Checklist is Not

• Not a style guide. Style is automated (linter, formatter).
• Not a gatekeeping tool. The goal is quality, not perfection.
• Not a substitute for the author's own checklist. The author runs agent-checklist.md first; this is the second pass.