Review Process & Communication

Scope discipline: architecture first, syntax last

The most common senior-review failure is inverted priorities: forty comments about naming on a component whose fundamental approach is wrong. If the architecture is wrong, every style comment is wasted on code that should not survive — and the author, having dutifully fixed forty nits, now feels entitled to a merge. Review in ordered passes and stop at the first pass that fails .

THE THREE-PASS REVIEW — stop at the first failing pass

  PASS 1: INTENT & ARCHITECTURE          (5 min, read the description
    Is this the right change?             and the shape of the diff)
    Does it fit the env's structure?
    ── fails? → one conversation, zero line comments. STOP.

  PASS 2: CHECKING INTEGRITY             (15-20 min, line by line on
    Constraints, timing, drain,           the artifact checklists)
    vacuity, bins, fork hygiene
    ── findings here are BLOCK comments.

  PASS 3: MAINTAINABILITY                (5 min)
    Naming, duplication, dead code,
    comments that contradict the code
    ── findings here are NOTES — author's choice unless repeated.

Pass 1 deserves emphasis: when the approach is wrong, the correct medium is a conversation (or a single summary comment proposing the alternative), not a line-by-line markup. Line comments on doomed code communicate 'fix these and merge' no matter what the summary says.

Comments that land

A review comment has two jobs: transmit the technical finding, and keep the author on your side for the next hundred reviews. Phrasing carries half the load.

Phrasing rules

• Question form for anything you are less than certain of: 'Can ready arrive in the same cycle as valid here? If so, does this wait hang?' — invites the author to either fix it or teach you, both wins.

• Severity tags on every comment: [BLOCK], [NOTE], [QUESTION], [NIT] — the author should never have to guess what gates the merge.

• Criticize the code's behavior, never the author's judgment: 'this drive races with the DUT sample' not 'you always forget clocking blocks'.

• Cite the rule, not your rank: link the checklist item, the spec section, or the past bug — 'see SVA checklist item 3 / bug #2241' beats 'I prefer it this way'.

• One positive observation per review when genuine — reviewers who only emit defects train authors to dread and delay review.

Block vs note: the line

• BLOCK: correctness of checking (anything from the artifact checklists), races, missing reset handling, unreachable stimulus space, vacuous assertions, silent drops — and architectural misfit per pass 1.

• NOTE: naming, structure preferences, optimization, style — even when you are right, these are the author's call this round; promote to BLOCK only if the team standard explicitly requires it.

• The test: 'if this merges as-is, can a bug escape or a future debug get materially harder?' Yes → block. No → note.

• Never bundle: a block buried in twelve nits gets the nit treatment. Lead with blocks, separate the rest.

Checklists as team artifacts, and mentoring through review

A checklist that lives in one senior's head dies with their attention span and walks out the door with their resignation. Make it a versioned team artifact: every escaped bug's post-mortem proposes a checklist line; every line cites the bug that earned it. The checklist then becomes self-justifying — when an author asks 'why does this matter?', the answer is a bug number, not an opinion.

Mentoring through reviews

• Reviews are the highest-bandwidth teaching channel a senior has — a finding explained well in review context sticks better than any training deck.

• Explain the failure mode, not just the fix: 'unchecked randomize keeps stale values, which passes today and corrupts the day a constraint conflicts' builds judgment; 'add an if' builds compliance.

• Calibrate depth to the author: a new grad gets the why behind every block; a senior peer gets terse tags and trust.

• Track repeat findings per author privately — three of the same finding means the lesson did not land; switch from comments to a pairing session.

• Let authors self-review against the checklist before requesting review — the checklist's real win is bugs that never reach the reviewer.

Key takeaways

• Review in three passes — intent, checking integrity, maintainability — and stop at the first failing pass.

• Tag every comment with severity; block only on correctness and architecture, and never bury blocks in nits.

• Question-form comments and cited rules transmit findings without spending relationship capital.

• Checklists earn lines from escaped bugs and live as versioned team artifacts — review is also how teams learn.

Common pitfalls

• Forty style comments on architecturally doomed code — wasted effort and a false promise of mergeability.

• Severity-free comments that force authors to guess what gates the merge.

• Pulling rank ('change it because I said so') where a checklist line or bug number would convince.

• Re-teaching the same finding in comments forever instead of escalating to a pairing session.