The review with order (the reminders feature's PR): the WHAT (description with scope), contract (no API changes), security (the reminder leaks nothing), correctness (the loop queries the fee per row: 09's N+1 — BLOCKER), tests ~ (the test misses the no-fee case — should), style (2 nits: variable name, import order). The classification: 1 blocker, 1 should, 2 nits — the "5 problems" PR was 1+1.
The mirror: reviewing style first: 7 style comments and the N+1 appeared at minute 25 (if it appeared) — the documented comparison: order is 80% of the review's effectiveness; style-first produces the panicked "LGTM with 7 nits" review.
The DNA caught what the normal review missed: the PR used cache.get of availability inside checkout (38 forbids it: 22 of the DNA "lock/decision in TX") — the "normal" review passed it (cache looks innocent); the DNA's question ("does the cache decide the purchase?") catches it.
Exercise 2 — The comments that teach
The rewrites:
"This is a horrible N+1."
→ "This loop fires 1 query per reservation: with 40 that's 83 queries (09).
prefetch_related takes it to 3 and the listing's test to green. Does it block your case?"
"Nobody does it this way."
→ "The repo centralizes errors in the handler (26): this view's try/except
duplicates the format. Moving to DomainError keeps the problem+json consistent."
"Why didn't you use X?"
→ "Did you consider select_for_update here? With two simultaneous requests the balance
can go negative (10). If there's a reason not to use it, shall we document why?"
"This test proves nothing."
→ "This test verifies `save` was called (the HOW): with 28's refactor it dies
without the behavior changing. Asserting the final state (status=CONFIRMED)
would prove the WHAT and survive."
"The legacy code was better."
→ "The previous version avoided the outbox in the same commit (25): if the atomicity
changed on purpose, does the outbox's atomicity test cover it? If not, the outbox's
rollback is lost — shall we review it?"
The question that teaches (the real finding): "What happens if two requests confirm the same intent at once?" → answered: the second passes the state machine if there is no lock (10) → the fix: select_for_update in confirmar_intent + 34's dispute test. The review's question was the whole review.
The honesty of marking: of the last PR's 9 observations: 2 real blockers, 3 shoulds, 4 nits — 4 written with blocker tone. The honest marking (nit: in front) would have saved the author the battle over the variable name and the conversation for what mattered.
Exercise 3 — The disagreement with data
The author's answer:
markdown
"I keep the direct manager for Venue/Seat: 25's ADR decides it (repository for
AGGREGATES with logic; manager for trivial CRUD). Venue has no invariants nor
locks: the repository would be an ORM wrapper with fewer features. If Venue grows
invariants, we open the repository with the test demanding it. 36's benchmark
(the browse plateau) doesn't change: the extra layer improves no numbers.
If you want, we reopen it with a new ADR (48): the standard is the document, not my taste."
The 4-step resolution plan: (1) identify the REAL discussion (repo standard or taste? — docs/review.md separates them); (2) if standard: the existing ADR decides (or the new ADR gets created with context and numbers); (3) if technical-uncertain: the 30-min benchmark/prototype (data decides, not volume); (4) the documented decision is respected in the PR and reviewed with its trigger — the review does not re-decide it every sprint.
The small PR: the 3 PRs (model+admin, service+tests, endpoint+contract) receive REAL reviews (the reviewer reads 400 lines with context vs 2000 with panic) — the change: the 2000-line PR receives "LGTM" (meaning "I didn't read it"); the 3 small ones receive the N+1 and the lock the big one hid.
Exercise 4 — The self-review
The last PR's self-review (32's saga): it failed 2 items: the scope without inside/outside ("resuming sagas" was not in the description) and the outbox's atomicity test did not enter the PR (left for "later" — the eternal later). The historically most-broken item: the self-review itself (not executed): the personal rule: the self-review IS EXECUTED (30 min) before the merge, not promised.
The diff with a stranger's eyes: the test test_saga_2 does not document the behavior (33's naming failed) → renamed to test_saga_reanudada_no_duplica_cargo; the PR description gained the hypothetical incident's "who/what/when". The stranger (47's you) now reads the diff without asking.
The team-of-1 virtual review: §5's checklist + the linter (41) + the prior ADR (48) + the new rule the exercise revealed: the 24 h wait (your own PR opened 24 h before your own merge: tomorrow's brain catches what the merge-day brain does not — "tomorrow's reviewer" is the only free second pair of eyes there is).
Exercise 5 — The review culture
The docs/review.md (excerpt): scale (blocker: contract/security/money · should: N+1/critical test · nit: style · question: dialogue) → §1's order → the two checklists (author/reviewer) → timings (review <4 h, re-review <2 h) → the disagreement rule (ADR or benchmark). One page.
The hard case: the author's answer: "Your proposed approach is valid but decides 3 unagreed standards (are they in the ADR?); the PR works and covers the case — I propose: merge with the current standards and the new ones go to the ADR/review.md for the next PR". The ideal reviewer: "Correct: my preferences are not standards until the repo adopts them — I open the ADR and the next PR carries them". The line: the standard exists if documented (ADR/review.md); if not, it is taste — and taste is proposed, not imposed.
The personal rule (excerpt): "I review in impact order and classify before writing; the comment carries fact+impact+path or a question; disagreement is settled with data or ADR, never volume; the self-review is executed, not promised."
Professor's summary
The review reads by impact (WHAT→contract→security→correctness→tests→style); the repo's DNAs are the 5 questions maintaining the system.
The grown-up comment: fact+impact+path, or a question; the scale (blocker/should/nit/question) avoids the nits' war.
The author answers everything and disagrees with data; disagreement closes with ADR or benchmark; the team-of-1's self-review is EXECUTED (and the 24 h wait is the second pair of eyes).
The Professor