The Consolidated Checklist

Transactions & constraints

1. Legality and typicality in separate, named constraint blocks — tests must relax 'typical' without losing 'legal'.

2. No hard constraints in base classes that encode typical traffic — they make corner cases unreachable for every derived class.

3. Every randomize() return value checked — failure keeps stale values and the test runs on garbage silently.

4. Response/status fields not rand (and not forced benign) on request objects — or error paths become untestable.

5. do_copy/do_compare/convert2string cover every behavioral field, and super.* is called — a missed field hides mismatches forever.

6. Names carry units and direction (addr_bytes, req_to_rsp_cycles) — unit confusion is a whole bug class.

7. Memory-map and mode facts come from config, not literals in constraints — hard-coded maps rot silently.

TB components

1. All pin drives and samples through clocking blocks — raw vif access on the active edge is a tool-dependent race.

2. Driver has a reset branch that aborts in-flight work and re-initializes pins — otherwise it abuses the DUT through reset.

3. item_done() only after the drive completes — early completion breaks the sequencer contract and overlaps pin traffic.

4. Monitors observe pins only, never driver state — driver-fed monitors verify the TB against itself.

5. Monitor handles the protocol's full legal timing range, not just current RTL behavior — RTL-shaped monitors break on legal RTL changes.

6. Scoreboard end-of-test checks BOTH expected-queue empty AND compare count > 0 — either alone permits a silent green.

7. No unbounded blocking waits — every get()/wait has a timeout or watchdog that names the starved path.

8. Every disable fork is isolated by an outer fork...join; every join_none has a documented owner and reset-kill story.

9. No try_put / silent-drop primitives on any checking path — drops must be loud.

Assertions

1. Every property has disable iff with the correct reset and polarity — ungated assertions cry wolf through every reset.

2. Every assert is paired with a cover of its antecedent — pass counts are meaningless without armed counts.

3. No contradictory or never-true antecedents — they pass vacuously forever (the cover pairing catches this).

4. Unbounded ##[1:$] / s_eventually only as documented liveness decisions; otherwise bounds cite the spec.

5. Properties sample on the right clock and domain — cross-domain sampling without synchronization is noise.

6. Assertion messages identify the violated rule and key values — 'assertion failed' is a debug tax on whoever hits it.

Coverage

1. Every bin name traces to a plan feature — unnamed auto-bins on multi-bit fields are explosion without intent.

2. illegal_bins for spec violations (hit = error), ignore_bins for out-of-scope-but-legal — swapped, they hide bugs or flag legal traffic.

3. Sampled once per completed transaction from the monitor stream — per-cycle or driver-side sampling inflates closure with junk.

4. Reset gated — reset-window samples pollute closure with invalid states.

5. Crosses filtered to plan-relevant combinations via binsof — full crosses on wide points are uncloseable.

6. per_instance set deliberately in multi-agent envs — one agent at 100% can mask another at 0%.

Tests & regression

1. Test passes are gated on activity (compares > 0, txns > 0), not just zero errors — silence must not be green.

2. Seeds and +args logged and reproducible from any failure line in one paste.

3. New bug fix lands with the test or assertion that would have caught it — otherwise the bug class survives.

4. Directed tests state their target scenario in a comment traceable to the plan.

5. Config knobs have poison defaults that fatal if never set — uninitialized knobs become one-in-a-thousand mysteries.

6. Watchdogs and drain logic report WHAT they are waiting for on timeout — hangs must self-diagnose.

Running the 30-minute review

THE 30-MINUTE REVIEW — clock-bounded, pass-ordered

  0:00–0:05  PASS 1: INTENT
             read PR description + diff shape; wrong approach?
             → stop, one conversation, no line comments.

  0:05–0:25  PASS 2: CHECKING INTEGRITY
             pick ONLY the checklist sections matching the diff:
               txn class touched?      → Transactions list
               driver/monitor/sb?      → Components list
               .sv with properties?    → Assertions list
               covergroups?            → Coverage list
               test/regression files?  → Tests list
             walk the diff once per relevant list, tag [BLOCK]s.

  0:25–0:30  PASS 3 + VERDICT
             quick maintainability scan → [NOTE]s;
             write summary: verdict, blocks first, notes after,
             one genuine positive if earned.

  Too big for 30 minutes? The PR is too big — say that and
  request a split. A 2-hour review of a 3,000-line PR finds
  fewer bugs than four 30-minute reviews of quarters.

• Use the relevant sections only — running all 34 items on a two-line diff is theater; the diff selects the lists.

• Print or pin the checklist; working from memory degrades to the three items you personally got burned by.

• After each escaped bug: add the line, cite the bug number, delete any line that has never fired in a year.

Key takeaways

• The checklist is organized by artifact so the diff itself selects which sections apply.

• Every line carries its rationale — items justify themselves with failure modes, not authority.

• Time-boxed, pass-ordered review beats heroic deep dives; oversized PRs get split, not endured.

• The list is alive: escaped bugs add lines with citations, and dead lines get pruned.

Common pitfalls

• Running the full list on every diff — checklist fatigue ends with no checklist at all.

• A checklist with no bug citations — it reads as one engineer's preferences and gets ignored.

• Accepting 3,000-line PRs and reviewing them heroically — defect-finding rate collapses with diff size.

• Letting the list grow forever without pruning — 80 items means nobody runs any of them.