The most common senior failure is inverted priority: forty naming comments on a change whose approach is wrong. Review in ordered passes and stop at the first one that fails.
THE THREE-PASS REVIEW — stop at the first failing pass
PASS 1: INTENT AND ARCHITECTURE (5 min, description plus diff shape)
Is this the right change, and does it fit the env structure.
Fails: one conversation, zero line comments. STOP.
PASS 2: CHECKING INTEGRITY (15-20 min, the artifact checklists)
Constraints, timing, drain, 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: the author decides, unless repeated.Line comments on doomed code promise a merge no matter what the summary says. When the approach is wrong, the medium is a conversation.
THE 30-MINUTE REVIEW — clock-bounded, pass-ordered
0:00-0:05 PASS 1: INTENT
read description plus diff shape; wrong approach
means one conversation and no line comments.
0:05-0:25 PASS 2: CHECKING INTEGRITY
run only the checklist sections the diff touches:
transaction class the transactions rows
driver or monitor the components rows
properties file the assertions rows
covergroups the coverage rows
test or seed config the tests rows
0:25-0:30 PASS 3 plus VERDICT
quick maintainability scan, then the summary:
verdict first, blocks before notes,
one genuine positive when earned.
Too big for 30 minutes: the change is too big. Ask for a split.
Four 30-minute reviews of quarters beat one 2-hour review.A review comment transmits the finding and keeps the author on side for the next hundred reviews. Phrasing carries half the load.
Question form below certainty: can ready arrive with valid here, and does this wait survive it. The author either fixes it or teaches you.
Severity tags on every comment: BLOCK, NOTE, QUESTION, NIT. The author never guesses what gates the merge.
Criticize the behavior, never the author: this drive races the DUT sample, not you always forget clocking blocks.
Cite the rule, not the rank: the checklist row, the spec section, or the past bug number.
One genuine positive per review. Reviewers who emit only defects train authors to delay review.
Comment | BLOCK | NOTE |
|---|---|---|
Correctness of checking: races, reset gaps, unreachable stimulus, vacuous asserts, silent drops | Yes | |
Architectural misfit found in pass 1 | Yes | |
Naming, structure taste, optimization, style | Yes | |
The test: a bug escapes or debug hardens if this merges | Yes |
Never bundle: a block buried in twelve nits gets the nit treatment. Lead with blocks and separate the rest.
The diff selects the rows: run only the sections the change touches. Every row carries its rationale, so items justify themselves by failure mode.
Area | Check |
|---|---|
Transactions | Legality and typicality in separate named blocks; tests relax typical without losing legal |
Transactions | No hard typical-traffic constraints in base classes; corners stay reachable |
Transactions | Every randomize return checked; failure keeps stale values otherwise |
Transactions | Response and status fields not rand on request objects; error paths stay testable |
Transactions | Copy, compare, and print cover every behavioral field, with super calls kept |
Transactions | Names carry units and direction; config, not literals, feeds maps and modes |
Components | All pin drives and samples off the active edge; raw edge access is a race |
Components | Driver aborts in-flight work on reset and rests the pins |
Components | Completion reported only after the drive finishes |
Components | Monitors observe pins only, across the full legal timing range |
Components | Scoreboard end of test needs empty queue and nonzero compares together |
Components | No unbounded waits; every wait has a timeout that names the starved path |
Components | Every disable fork isolated; every fork has an owner and a reset story |
Components | No silent drops on any checking path |
Assertions | Every property reset-gated with the correct polarity |
Assertions | Every assert paired with a cover of its antecedent; armed counts reviewed |
Assertions | No contradictory or never-true antecedents |
Assertions | Liveness bounded with a spec cite, or an explicit documented decision |
Assertions | Properties sample the right clock and domain |
Assertions | Failure messages name the rule and the key values |
Coverage | Every bin traces to a plan feature; no bare auto-bins on wide points |
Coverage | Illegal marks spec violations that must raise; ignore marks legal out-of-scope |
Coverage | Sampled once per transaction from the monitor stream, gated off reset |
Coverage | Crosses filtered to plan-relevant combinations |
Coverage | Per-instance set deliberately where agents could mask each other |
Tests | Passes gated on activity counts, not on zero complaints alone |
Tests | Seeds and arguments logged and reproducible from any failure line |
Tests | Each fix lands with the test or assertion that would have caught it |
Tests | Config knobs default to values that raise loudly when never set |
Tests | Watchdogs report what they waited for on timeout |
A checklist in one senior head walks out with the resignation. Keep it versioned: each escaped bug proposes a row with its bug number, and rows that never fire get pruned.
Explain the failure mode, not just the fix. Judgment transfers; compliance does not.
Calibrate depth to the author: full whys for new engineers, terse tags for senior peers.
Repeat findings mean the lesson did not land. Switch from comments to a pairing session.
Ask authors to self-review against the list first. The real win is bugs that never reach a reviewer.
One conversation about the approach, with zero line comments. Line markup on doomed code promises a merge the author will expect.
A row demanding the antecedent cover that would have caught it, citing the bug number, plus the missing cover added to the file itself.