Skip to content

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

TimeWhat
0–3 minRe-state the moment. Author reads the story aloud and the predicted scenarios. The reviewer hears the framing before the diff.
3–20 minWalk 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 minThe negative case walk. Where does this code fail? Empty input, network drop, concurrent submit, permission revoked. For each: is the behaviour tested?
27–30 minDecision. 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

200apps · How We Work · NWIRE