Chapter 45 — Independent design review
Lesson map
Lesson 45 published. Map, example, EX133.
flowchart TD
N0["Learning outcomes"]
N1["45.1 Review rights"]
N2["45.2 Checklist"]
N3["45.3 Dissent"]
N4["45.2b Reviewer preparation"]
N5["45.3b Review SLOs"]
N6["45.1b External reviewers"]
N7["Practice note"]
N0 --> N1 --> N2 --> N3 --> N4 --> N5 --> N6 --> N7
N7 --> EX["Worked example"]
EX --> Q["Exercise EX133"]
Logical drawing
flowchart LR
N0["Learning outcomes"]
N1["45.1 Review rights"]
N2["45.2 Checklist"]
N3["45.3 Dissent"]
N4["45.2b Reviewer preparation"]
N5["45.3b Review SLOs"]
N6["45.1b External reviewers"]
N7["Practice note"]
N0 --> N1 --> N2 --> N3 --> N4 --> N5 --> N6 --> N7 --> Q["EX133"]
[Learning outcomes]
|
v
[45.1 Review rights]
|
v
[45.2 Checklist]
|
v
[45.3 Dissent]
|
v
[45.2b Reviewer preparation]
|
v
[45.3b Review SLOs]
|
v
[45.1b External reviewers]
|
v
[Practice note]
|
v
[Worked example] --> [Exercise EX133]
Learning outcomes
You will (1) run a review that can reject; (2) use a checklist tied to safety requirements; (3) record dissenting opinions.
45.1 Review rights
Reviewers may block merge for fail-open paths, missing tests, or empty scores in UI. "LGTM" without running the checklist is invalid.
Checklist-free LGTM is how fail-open returns under deadline. Sam's coverage-badge PR collected two blocks under this rule before the honest UI shipped. A third block would have been fine; speed is not a reason to skip the checklist. Authors may not mark their own checklist boxes; reviewers must. A block must name the checklist item that failed, so the author knows what to fix.
45.2 Checklist
Fail-closed; policy authn; schema forbids payloads; coverage has denominator; secrets redaction; rollback path documented.
The six lines are merge gates, not suggestions. Fail-closed and denominator rules map to earlier chapters; secrets redaction and rollback close the operations loop. Sam's review bot posts the checklist and refuses approve until each box has a link to evidence in the PR. Links to tests beat checkmarks typed by hand with no URL. A box without a link is treated as unchecked by the bot.
45.3 Dissent
If one reviewer disagrees, record the dissent in the ADR even if overruled — future incidents need that trail.
Synthetic case — Riverstone Transit. A coverage badge PR was blocked twice under this checklist; the third revision shipped the honest UI (Chapter 35).
Overruled dissent still belongs in the ADR for the incident that may come later. The coverage-badge blocks are recorded with the minority view that a badge could return through a flag. Sam's third revision removed the flag path entirely so the dissent was resolved by deletion, not by override.
45.4 Errors and limits
Reviews after merge are archaeology, not control.
Commenting after merge cannot stop a bad binary that already left the shed. Review is a control only while the button is still red. Sam's team treats post-merge comments as new tickets, never as pretend review of the shipped tag. Shipped tags are immutable; fixes land as new versions with new reviews. Hotfixes get the same checklist on a compressed SLO, not a waiver. Compressed SLO still requires every checklist link to be present.
45.2b Reviewer preparation
Reviewers run the critical tests locally on synthetic fixtures before the meeting — not only read the diff. A review that never executed fail-closed is incomplete regardless of comment volume.
Unread fail-closed tests are theatre with comments. Running them on synthetic fixtures before the meeting is the preparation bar. Sam's review SLO clock does not start until the reviewer pastes the local test output into the PR. Pasted output includes the fail-closed row names so skimming is obvious. Meeting time is for judgment calls, not for first-time test runs.
45.3b Review SLOs
Reviews complete within an agreed number of business days or the change auto-expires from the merge queue (not auto-merges). Pressure to rubber-stamp is a safety smell.
Auto-expire without auto-merge removes rubber-stamp pressure. A change that ages out must be refreshed and re-reviewed. Sam's queue expired a scope-touching PR on day five; the author fixed the checklist gaps before it returned. Refreshing an expired PR restarts the SLO clock from zero. Expired PRs cannot be force-pushed green without a new reviewer assignment.
45.1b External reviewers
Once per term, invite an engineer from outside the squad to review a safety-critical PR. Fresh eyes catch normalized deviance ("we always skip that test").
Normalized deviance such as "we always skip that test" needs a stranger to hear it. One external review per term on a safety-critical PR is the minimum. Sam's external reviewer caught a missing invalidation test on the policy cache that internal review had waved through. The invalidation gap became a Lab II case the following week.
Practice note
Review culture forbids merging on Friday evenings before engagement weekends unless the change is a safety hotfix with Lab I evidence attached. The calendar is part of the control design; heroic late merges are a smell, not a badge.
Rotate checklist owners each month so review quality does not depend on one conscientious engineer.
Blocking comments must cite a requirement id or checklist item number.
Checklist item numbers appear in the PR template as a required fill-in list.
Repeated checklist skips by the same reviewer trigger a coaching conversation, not a silent reassignment forever.
45.4b Closing review comments
Every review comment ends in one of three states: fixed, with a link to the change; declined, with a reason the reviewer accepts; or carried as a known risk with an owner. Sam's design is approved only when no comment remains open without one of these states.
Carried risks need owners. Sam's approval on the honest coverage UI waited until every badge-related comment was either fixed or declined with a reason the reviewer accepted. Comments carried as known risks are copied into the risk register with their owner and a review date.
Exercises
- EX133 — Block. Write a blocking comment for a fail-open retry. Rubric: 3 = cites safety req + fix ask; 2 = vague block; 1 = nit; 0 = approve.
- EX134 — Checklist. Add one item for radio engagements. Rubric: 3 = specific; 2 = vague; 1 = minimal attempt; 0 = none.
- EX135 — Dissent. One sentence of recorded dissent. Rubric: 3 = clear; 2 = hedged; 1 = vague; 0 = none.
Synthesis
Reviews that can say no, with a checklist and a memory.
Continue studying — chapter 45 of 50
← Chapter 44 (draft) · Course chapter index · Chapter 46 (draft) →