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?

If GitOps + Operators

• Is desired state sourced from git, not from the cluster?
• Is configuration declarative, not imperative scripts?
• Is reconciliation pull-based (no external push credentials into the cluster)?
• Does the reconciliation loop run continuously and auto-correct drift?
• Is every change a commit, with history as the audit/rollback path?
• Is operational knowledge encoded as CRDs/controllers, not runbooks humans must remember?
• Does progressive delivery (canary/blue-green) have a tested abort/rollback path?
• Are there manual kubectl apply/kubectl edit on GitOps-managed resources? (flag as incident)
• Do sync failures, health degradation, and rollout stalls emit status + notifications?
• Are controller credentials scoped to reconciled namespaces/resources (no cluster-admin GitOps robot)?

If AI / ML

• Scope check: is this engineering discipline (data versioning, evaluation, serving, drift), NOT algorithm/model design? (D-023 — reject algorithm-design content)
• Is every training run reproducible from pinned data + code + config + environment?
• Are datasets, features, and splits versioned artifacts with lineage (not just git)?
• Can any deployed prediction trace back through model → training run → dataset → source?
• Are metrics, splits, and thresholds declared a priori (no post-hoc metric cherry-picking)?
• Are models pinned, immutable, registry-tracked artifacts (never "the latest")?
• Is inference observable (latency, throughput, input distributions, prediction confidence)?
• Are data drift, concept drift, and prediction drift monitored (drift signal = incident)?
• Are inference inputs validated against the model's contract (schema, ranges, types)?
• Are training/serving flows composable pipelines (not notebooks in production)?
• Does serving rollback restore the prior model artifact, not just the prior code?

If i18n

• Is the source language treated as one locale among many, not the "neutral" default?
• Do locale identifiers use BCP 47 tags (no ad-hoc locale codes)?
• Are user-facing strings in locale resource files (not concatenated inline in code)?
• Do plural/gender/select use ICU MessageFormat (no if (n == 1) branching)?
• Are dates, times, numbers, currencies, units formatted via ICU/CLDR/Intl (no hand-rolled formatters)?
• Is RTL/bidi a first-class layout concern (logical CSS properties over physical)?
• Do layouts accommodate translation expansion (no fixed pixel widths for text)?
• Are pseudo-locales used to test before real translations arrive?
• Are icons, colors, and imagery reviewed for locale-sensitivity?
• Are resource files versioned (bad translation = rollback, not hot-patch)?

If Compliance

• Scope check: is this framework-agnostic (no regulation-specific GDPR/HIPAA/SOC2/PCI content)? (D-024)
• Are audit records immutable once written (deletion/mutation is itself an auditable incident)?
• Is the set of auditable actions defined a priori ("we forgot to log it" is a violation)?
• Is data lifetime declared and enforced as policy (deletion at end-of-life is a feature)?
• Is compliance policy expressed in versioned, reviewable, testable code (not spreadsheets/prose)?
• Do policy violations block before the action (admission/CI/CD-time, not after the audit)?
• Is evidence gathered as a byproduct of operation (not assembled manually at audit time)?
• Does every logged action trace to an authenticated principal (no shared/generic identities)?
• Are data-subject rights (access, export, deletion) operations with defined contracts and audit trails?
• Do audit logs avoid leaking secrets (redaction is structural, not opportunistic)?
• Does the system report its own compliance state (drift from policy, open violations, retention status)?

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.