session · build & review
Code review session
30 minutes. Sync, called when async review stalls. Author plus reviewer walk the diff with the brief beside them. The trace question, lived: does this code carry the meaning the chain handed in?
When
- When an async PR review stalls past 24 hours — the author calls the session rather than continuing comment-by-comment.
- When the PR is high-risk — migration, security, integration with a new service. Sync from the start; don't waste a day on async.
- When the reviewer flags a structural concern — better in conversation than in a thread.
Who
- The PR author — drives the screen.
- The reviewer — asks the trace question and the chain-aware questions.
- (Optional) Tech Lead if the change touches architecture; Designer if the change affects the surface.
Time-box
30 minutes. If a sync review needs more than 30 minutes, the PR is too big — split it, or the chain handed the developer a story that wasn't ready.
Inputs
- The PR diff.
- The Feature Brief and the story (with Gherkin scenarios).
- The ADR(s) the code is built against, if any.
Agenda
| Time | What |
|---|---|
| 0–3 min | Re-state the moment. Author reads the story aloud and the predicted scenarios. The reviewer hears the framing before the diff. |
| 3–20 min | Walk the diff in the order the user encounters the behaviour — entry point first, edge cases last. Author narrates; reviewer asks the trace questions: Does this code use the brief's words? Does the implementation honour the ADR? Are all the amigos scenarios covered? Are all named states implemented? |
| 20–27 min | The negative case walk. Where does this code fail? Empty input, network drop, concurrent submit, permission revoked. For each: is the behaviour tested? |
| 27–30 min | Decision. Approve, request changes (with named items), or schedule a follow-up. Verbal approval still requires the GitHub/Bitbucket approval — the artefact carries the record. |
Outputs
- PR state — approved, changes requested, or scheduled for follow-up.
- Action items, if any — written into the PR thread, not just discussed.
- (If approved) the merge happens within the session if green; never as a vague "I'll merge later."
What good looks like
The reviewer names what they checked — not just what they found. "Read the story, read the four scenarios, read ADR-014. Handler is thin, domain function is named, three scenarios covered. Missing test for the permission-revoked-mid-flow case." That is review. "LGTM" is not.
The session ends with a smaller, clearer PR — either merged, or with named items the author can ship in an hour.
Anti-pattern
The session becomes a rewrite negotiation. Reviewer wants restructuring; author defends. The conversation runs 45 minutes; no decision reached; both leave annoyed. Fix: if the structure is wrong, the ADR was wrong — that is a follow-up ADR, not a PR rewrite. Most "rewrite this PR" disputes trace to a missing ADR review.
A second anti-pattern: the reviewer pre-decided to approve and the session is performative. The PR has subtle issues but the reviewer doesn't engage. Fix: if you have nothing to ask, approve in 2 minutes asynchronously; the sync session is for review that actually reviews.
See also
- Canon — How We Build · Code review as chain verification · The Review
- Area — Code Review
- Adjacent session — Three-confirmation review — the broader trio sync before merge