DEV Community: Othman Shareef

Trunk Based Development Code Review: Keep It Fast

Othman Shareef — Thu, 10 Sep 2026 08:00:00 +0000

Trunk-based development has a reputation for being anti-review, and it is unearned. The canonical reference describes a model where developers collaborate in a single branch called trunk, resist long-lived development branches, and commit multiple times a day. But it also explicitly blesses short-lived feature branches for exactly two purposes: code review and build checking. Trunk based development code review is not an afterthought bolted onto the model. It is the model, with one non-negotiable property: it has to be fast.

The short answer: Trunk-based development does not remove code review; it constrains it until review stops being a bottleneck. Branches live hours or a day or two, so diffs stay small. Diffs stay small, so reviews finish fast. Reviews finish fast, so branches can stay short. Feature flags decouple deploying code from releasing features, which removes the last excuse for long branches. Break any link in that loop, usually review speed, and the whole model quietly reverts to feature branches.

What the model actually prescribes

Strip away the folklore and the prescription is concrete. Everyone integrates with trunk at least daily, which is the bar Continuous Integration has always technically demanded. Branches, where they exist at all, live for hours or a couple of days, exist so that a reviewer and a build server can check the change before it lands, and are never the place where releases get cut. A build server verifies every commit to trunk, because with everyone integrating constantly, a broken trunk blocks the whole team. None of that abolishes review. It relocates review to the only place it can keep up: small changes, checked quickly, merged the same day.

Small PRs are the review model, built in

Most teams fight an endless battle to keep pull requests reviewable. Trunk-based teams get that property structurally: if every branch integrates within a day or so, a PR physically cannot grow to two thousand lines. The change is one coherent step, which is exactly the shape review works best on. We have made the case for small PRs on review quality grounds, and Google makes it on velocity grounds in their small CLs guide: small changes are reviewed faster, more thoroughly, and with less wasted rework when the design is wrong. Trunk-based development takes that advice and makes it mandatory instead of aspirational. The branching model is doing the PR-size policing your process documents never managed to do.

Trunk based development code review must be fast or the model dies

Here is the failure mode: a developer cuts a short-lived branch Monday morning, opens a small PR by noon, and the review sits until Wednesday. Now they either start the next change on top of unmerged work, recreating the stacked, drifting state the model exists to prevent, or they stall. Multiply by a team and trunk-based development degrades into feature-branch development with extra steps. Review latency is the load-bearing number. Google’s reviewer speed guide draws the line at one business day for a first response, and frames slow review as a team-velocity problem, not a reviewer-convenience problem. Trunk-based teams need to treat that as a ceiling, not a target, which is why they benefit most from explicit review SLAs. Tooling matters here too; ours (Pyor) exists largely because a reviewer who gets the diff organized by what matters first can turn a review around in the window trunk-based development actually allows.

Feature flags decouple deploy from release

The classic argument for long-lived branches is hiding unfinished features until they are ready. Trunk-based development answers with feature flags: merge the incomplete code, keep it dark behind a flag, and release by flipping configuration rather than by merging a branch. The canonical guidance pairs this with branch by abstraction for longer structural changes, and describes flags as a way of hedging on the order of releases. For review, this is a quiet win. The reviewer sees small, integrated slices of a feature as they land, instead of one giant reveal at the end. The cost is real: flags are code, they accumulate, and unflagged cleanup is a review item of its own. But reviewing ten small flagged PRs beats reviewing one thousand-line merge every time.

When review becomes post-commit sampling

Some mature trunk-based teams go further: commit straight to trunk, review after the fact, sometimes only a sample. That is a legitimate end state, not a cheat, but it is a different contract. Pre-merge review is a gate; post-commit review is monitoring, the shift we described in human in the loop vs on the loop. The honest prerequisites: a build server that verifies every commit, deploys that are easy to revert, blast-radius awareness about which paths still get pre-merge eyes, and an actual sampling discipline rather than review quietly stopping. Auth, payments, and data migrations should stay gated even when everything else flows. Post-commit sampling is what review looks like when trust and automation are both high. It is earned, and it is reversible the moment defect rates say so.

The loop to protect

Everything above is one feedback loop. Short branches keep diffs small; small diffs keep reviews fast; fast reviews keep branches short; flags keep unfinished work merged instead of hidden. Protect the loop at its weakest link, which in most organizations is reviewer turnaround, and trunk-based development delivers the thing it promises: integration as a habit rather than an event. Let review latency creep, and no amount of branching policy will save you; the long-lived branch will come back wearing a different name.

Frequently asked questions

Does trunk-based development eliminate code review?

No. The trunk-based playbook explicitly allows short-lived feature branches whose purpose is code review and CI checking before the code integrates into trunk. What it eliminates is long-lived branches and the giant, week-old PRs they produce. Review survives, but it has to be small and fast enough not to become the new long-lived branch in disguise.

How fast does review need to be for trunk-based development?

Faster than your merge cadence. If developers integrate at least daily, a review that waits two days forces either queued work or drift, both of which break the model. Google’s reviewer guide sets one business day as the outer bound for a first response, and trunk-based teams should treat hours, not days, as the working norm.

What is post-commit review?

Code merges to trunk first and gets reviewed after, either every change or a sampled subset. It trades the gate for throughput and works only with strong automated checks, easy reverts, and a real sampling discipline. It shifts the reviewer from approving each change up front to monitoring the stream and intervening when something looks wrong.

Pair Programming vs Code Review: Not Rivals

Othman Shareef — Tue, 08 Sep 2026 08:00:00 +0000

Every few months the pair programming vs code review debate resurfaces, usually framed as a choice: if two people wrote the code together, why review it again? The framing is wrong. Pairing and review both put a second brain on the code, which makes them look interchangeable from a distance, but one is synchronous co-creation and the other is asynchronous verification. They catch different classes of problems at different points in time, and the teams that get the most out of either tend to run both.

The short answer: Pair programming and code review are not substitutes. Pairing is synchronous co-creation: it catches design missteps in the moment, before they harden into structure. Review is asynchronous verification: it adds fresh eyes that were absent during writing, plus a durable record of what was decided and why. Some trunk-based teams replace review with pairing, but that trade has real requirements. Most teams should pair on the gnarly work and review everything else.

Pair programming vs code review: different jobs

Pairing is a writing practice. Two people share one problem in real time, and the navigator questions the approach while it is still cheap to change. Review is a reading practice. Someone who was not in the room reconstructs the change from a diff, later, with no shared context to lean on. The research reflects that split: Microsoft’s study of modern code review (Bacchelli and Bird) found that although defect finding is the top stated motivation for review, the observed outcomes lean heavily toward code improvement, knowledge transfer, and team awareness. Those are reader-side benefits. Pairing delivers its value on the writing side, before a diff even exists.

What pairing catches: design missteps, early

The cheapest moment to catch a design mistake is before code accumulates on top of it, and that is pairing’s home turf. A navigator asking “why a queue here?” at minute ten saves the three days it would take to unwind that decision in review, where the same objection arrives after the structure has hardened and the author is defending sunk cost. Pairing also transfers tacit knowledge continuously: debugging habits, tooling tricks, the unwritten reasons the codebase looks the way it does. What pairing does not deliver is fresh eyes. By the second hour, both people share the same context and most of the same blind spots. A pair can talk itself into a bad idea just as smoothly as an individual can, sometimes more smoothly, because agreement feels like validation.

What review catches: fresh eyes and a record

A reviewer arrives cold, and that is the point. They read the change the way a future maintainer will: without the conversation, without the context, without knowing which alternatives were already rejected. That coldness surfaces what pairing structurally cannot: the missing comment, the name that only makes sense if you were there, the edge case both partners stopped seeing. The evidence on raw defect discovery is more modest than folklore suggests (we have covered what reviews actually find), but the fresh-eyes read reliably catches comprehension problems, and comprehension problems are what kill codebases slowly. Review also leaves an artifact. The thread of comments, objections, and resolutions is a durable, searchable record of why the code is the way it is. Pairing produces better code and no trace.

When pairing replaces review

Some trunk-based teams treat pairing as the review: the code was continuously inspected while it was written, so it merges to trunk without a second gate. That model is real and it can work, but its requirements deserve honesty. First, the pairing has to be genuine: two engaged engineers rotating roles, not a senior typing while a junior watches. Second, rotation across pairs has to be systematic. Without it you trade individual silos for pair-shaped ones, and nobody outside the pair ever reads the code, so the fresh-eyes check never happens at all. Third, there is no written record; if your process commitments require documented review (the same forces that push teams toward review SLAs usually require the paper trail too), pairing alone will not satisfy them. Teams that drop review without meeting those bars are not making a disciplined trade. They are simply not reviewing, which is at least more honest than LGTM theatre, but no safer.

The hybrid most teams should run

Pair on the gnarly work: novel design, unfamiliar territory, risky migrations, anything where a wrong early decision is expensive to unwind. Review the rest asynchronously, where the interruption cost of scheduling two people is not justified. When a pair does open a PR, review it lighter, not zero: a fast comprehension pass from outside the pair, not a line-by-line audit of code that already had two authors. Google’s Critique case study is instructive here: even with a mature review culture and heavy tooling, Google keeps review universal partly for education, because reading each other’s changes is how standards propagate. Pairing spreads knowledge deep between two people; review spreads it wide across the team. You want both directions, and neither practice gives you the other one for free.

If you need a tiebreaker for a given piece of work, price the two honestly. Pairing costs two synchronized calendars for the duration of the work; review costs latency and a context switch, but the reviewer schedules it themselves. High-uncertainty work justifies the synchronous price because the feedback loop is measured in seconds. Routine work does not, and forcing pairing onto it breeds the checked-out navigator that gives the practice a bad name. Pick per task, not per ideology, and let the two practices cover for each other’s blind spots.

Frequently asked questions

Does pair programming replace code review?

Sometimes, but only under real conditions: engaged role rotation within the pair, systematic rotation across pairs so knowledge spreads, and no compliance requirement for a documented review. Some trunk-based teams meet those bars and merge pair-authored code without a second gate. Most teams do not, and for them pairing plus a lightweight review works better than picking one.

What does code review catch that pairing misses?

Fresh-eyes problems. A pair shares context and blind spots by the second hour, so neither partner notices what only makes sense if you were there: unclear names, missing context, undocumented assumptions. A cold reviewer reads the change the way a future maintainer will and surfaces those comprehension gaps. Review also leaves a searchable record of decisions, which pairing never produces.

Should a pair-authored PR still be reviewed?

Usually yes, but lighter. The design was already challenged in real time by the navigator, so a line-by-line audit mostly duplicates work. A quick pass from someone outside the pair adds the one thing pairing structurally cannot: a reader with no shared context. It also creates the written record your future team will search for.

Async Code Review for the Distributed Team

Othman Shareef — Sun, 06 Sep 2026 08:00:00 +0000

On a co-located team, a lazy review comment costs five minutes: “why is this a map?” gets answered across the desk and everyone moves on. Async code review on a distributed team reprices every one of those exchanges. With a nine-hour gap, the same throwaway question lands after the author has gone home, gets answered while the reviewer sleeps, and the clarification arrives a full calendar day after the diff was ready. Nobody was slow. The process was.

The short answer: Distributed code review is a round-trip minimization problem. You cannot make a reviewer nine time zones away respond faster, so the only lever is needing fewer exchanges: front-load context so the first pass needs no orientation questions, batch feedback into one complete review, hand off explicitly at end of day, and spend scarce overlap hours only on threads that need real conversation.

Async code review is a round-trip problem

Google’s review guidance sets one business day as the maximum response time, and on a distributed team that ceiling becomes the floor: with little or no overlap, every exchange takes roughly a day no matter how disciplined people are. That changes the arithmetic of everything else. A review that takes four round trips is not four small delays; it is most of a week. So the metric worth optimizing in async code review on a distributed team is not response latency, which geography has fixed for you, but exchange count. Every practice below is the same idea wearing different clothes: turn two round trips into one, or one into zero.

Front-load the context

The first round trip on most PRs is pure orientation: what is this, why now, where do I start reading. On a distributed team that costs a day, and it is entirely avoidable, because the author already knows the answers. The discipline of author self-review pays double across time zones: annotate your own diff before requesting review, flag the risky part, say what you are unsure about, explain the approach you rejected. For AI-assisted changes this is even more binding, since the reasoning lives in a session that will be gone by morning; capture the intent while it exists. A description the reviewer can trust is the cheapest day you will ever buy back.

Batch comments into complete reviews, never dribble

Commenting as you read is a habit formed in offices, where each comment costs the author a glance. Async, each dribbled comment is potentially its own day-long round trip, and a review that arrives in three installments can eat three days before the author even knows the verdict. The rule for distributed teams is absolute: read the entire diff, then submit one review that is complete on its own terms. Complete means it states an explicit outcome (approve, approve with nits, changes requested), separates blocking issues from suggestions, and asks every question in the same pass. It also means each comment carries enough context to be actionable without a follow-up: the craft covered in review comments that land. A comment that needs a clarifying question before the author can act on it is, across time zones, a two-day comment.

End every day with a handoff note

The silent killer in follow-the-sun review is ambiguous state: the author pushed fixes but did not say which comments they address; the reviewer looked again but did not say whether they are done. The other side wakes up, cannot tell whose move it is, and a day evaporates on nothing. The fix is a norm, not a tool: whoever touches the PR last before signing off writes one summary comment. Addressed 1 through 4, pushed in the latest commits; disagree on 5, reasoning inline; still need your call on the migration ordering. Thirty seconds of writing, and the counterpart starts their day with a move to make instead of an investigation. Teams that adopt this one habit routinely drop a full round trip per PR. (Full disclosure: making that wake-up triage fast is exactly why our own tool, Pyor, has a comments inbox; but the norm works in any tool, including plain GitHub.)

Record walkthroughs for the big ones

For a large or architecturally novel change, the first review pass is mostly the reviewer reconstructing your mental model, and their orientation questions are another day gone. A five minute screen recording collapses that: walk the diff in the order it should be read, name the core decision, point at the two places you want real scrutiny. The reviewer watches it at the start of their day and begins from understanding instead of archaeology. This is not a substitute for reading the code; it is a substitute for the round trip where the reviewer asks you where to start.

Budget overlap hours for the contentious threads

Some disagreements should not be settled async. A design dispute that has gone two rounds in comments will not converge on round three; it will generate a week of increasingly formal paragraphs. Overlap hours, if you have even one or two, are the scarcest resource a distributed team owns, and they should be spent exactly there: fifteen minutes of synchronous conversation resolves what five async rounds cannot. The practical rule is a two-round cap. Any thread still open after two exchanges gets pulled into the next overlap window, decided, and summarized back onto the PR for the record. Everything else (nits, questions with factual answers, mechanical fixes) stays async, where it belongs. Distributed review fails when teams treat every thread the same; it works when the expensive channel is reserved for the threads that need it.

Frequently asked questions

How do you speed up code review across time zones?

Cut round trips, not response time. A reviewer nine hours away cannot respond faster, so every exchange costs a day regardless of diligence. Front-load context in the PR description, deliver complete batched reviews instead of dribbled comments, end each day with explicit handoff notes, and reserve overlap hours for the threads that genuinely need back-and-forth.

Should distributed teams comment as they read or batch feedback?

Batch, always. Dribbled comments are tolerable when the author sits nearby and can answer in minutes; across time zones each dribble is its own 24 hour round trip. Read the whole diff, then submit one complete review that states its verdict, separates blocking from optional, and asks every question you have in a single pass.

When is a recorded walkthrough worth making for a PR?

When the change is large, architectural, or likely to be misread: a five minute screen recording walking the diff in reading order can replace the first full round trip, which on a distributed team is a full day. For routine PRs a good description is enough; recordings earn their cost on the changes where the first review pass would otherwise be spent asking orientation questions.

Code Review Metrics Worth Tracking (and the Traps)

Othman Shareef — Fri, 04 Sep 2026 08:00:00 +0000

Code review metrics have a bad reputation for a good reason: most teams that adopt them either track vanity numbers that measure typing, or wire real numbers into dashboards that people immediately learn to game. Both failure modes are avoidable. There is a short list of review metrics that reflect things worth caring about, and one rule for using them: they are conversation starters, not scorecards. Here is the list, and the trap attached to each.

The short answer: Four code review metrics carry most of the signal: time to first response, review cycle count, PR size distribution, and rubber-stamp rate. Each one measures a real failure mode, and each one becomes theatre the moment it turns into an individual target. Track distributions at team level, discuss the outliers, and never pay anyone by the number.

Time to first response

The single highest-value number, because waiting is the dominant cost of review. Google’s engineering practices set the reference point: one business day is the maximum to respond to a review request, with the guidance treating review speed as a property that shapes the whole team’s velocity. Measure the distribution, not the average: a two-hour median with a three-day tail is a routing problem (lost notifications, absent reviewers), while a uniformly slow median is a capacity problem. The two look identical in an average and require opposite fixes. If you formalize expectations here, do it as a responsiveness SLA, not a completion deadline.

The trap: target the number individually and reviewers discover the placeholder comment. “Looking!” stops the clock and helps nobody.

Review cycle count

How many author-reviewer round trips a PR takes before merge. One or two cycles is a healthy conversation; five is a symptom. High cycle counts almost always decompose into one of three causes: reviews arriving as a dribble of partial comments instead of one complete pass, unclear PR descriptions that force discovery through comment threads, or genuine design disagreement that should have happened before the code was written. The metric will not tell you which, and that is fine. Its job is to flag the PRs worth asking about. A useful refinement is to segment by change type: refactors and migrations legitimately take more rounds than feature work, so a single blended average hides more than it shows.

The trap: push the number down as a goal and reviewers start swallowing legitimate second-round concerns to avoid “causing” another cycle.

PR size distribution

Size is the upstream variable that drives everything else. SmartBear’s review research found defect detection falls off past roughly 400 lines of code, with inspection rates above about 500 lines per hour degrading review effectiveness. So a team whose PR size distribution is drifting upward is watching its review quality decay in advance. Plot the distribution monthly; the interesting number is the share of PRs over the threshold your team can actually review well, and what a right-sized PR is depends on the kind of change. A creeping tail of giant PRs predicts every other metric on this page getting worse a quarter later.

Rubber-stamp rate

The share of approvals with zero comments on non-trivial diffs. Some silent approvals are legitimate (a rename, a dependency bump, a change discussed in person), which is why the qualifier matters: filter to diffs above a size floor and outside mechanical categories, then look at what is left. A rising rubber-stamp rate is the quantitative shadow of LGTM culture: reviews still happen on paper while reading has quietly stopped. This one is the best early-warning metric of the four, because it moves before defect rates do.

The trap is the mirror image of the others: make “comments per review” a virtue and you get nitpick theatre, reviewers seeding trivial style comments to prove they read the diff. The point is not more comments; it is honest ones.

Code review metrics meet Goodhart’s law

Every metric above stops working when it becomes a target, which is Goodhart’s law doing exactly what it always does. The pattern repeats: turnaround targets produce placeholder responses, cycle targets suppress real feedback, comment targets produce noise. This is not an argument against measurement; it is an argument about placement. Keep code review metrics at the team level, review them monthly as distributions and trends, and treat every surprising movement as a question: what changed, which PRs drove it, what would we like to try.

It also helps to name the metrics you refuse to track. Comments per reviewer rewards noise. Lines reviewed per day rewards skimming. Defects caught per reviewer punishes people for reviewing clean code and starts arguments about what counts as a defect. Approval rate per person turns reviewers into either rubber stamps or gatekeepers depending on which direction the dashboard frowns. Each of these measures activity, and activity is the one thing reviewers can manufacture on demand.

Metrics are conversation starters, not scorecards

The operating model that keeps all four numbers useful: pull them monthly, put them in front of the team, and ask what they make people curious about. A metric that prompts “why did our first-response tail double in March” is doing its job. A metric that decides someone’s rating has already been gamed, whether or not anyone admits it. Start with time to first response and rubber-stamp rate if you adopt only two, since together they answer the questions that matter most: do reviews start, and are they real. The numbers are instruments for noticing; the conversation is where the improvement actually happens.

Frequently asked questions

What are the most useful code review metrics?

Four cover most of the signal: time to first response, review cycle count per PR, PR size distribution, and rubber-stamp rate, meaning approvals with zero comments on non-trivial diffs. Together they show whether reviews start promptly, converge quickly, receive right-sized work, and involve real reading. Track them as team-level distributions, not individual scores.

Should code review metrics be used in performance reviews?

No. The moment a review metric feeds an individual scorecard, Goodhart takes over: people optimize the number, not the outcome it was meant to reflect. Comment counts breed nitpick theatre, turnaround targets breed instant rubber-stamps. Keep metrics at team level and treat movement as a question to investigate, never as a verdict on a person.

What is a good time to first review response?

Google’s engineering guidance sets one business day as the maximum, and healthy teams sit far inside it, with medians measured in hours. The useful practice is watching your own distribution rather than chasing a universal number: a stable median with a shrinking tail means routing works, while a growing tail points at lost requests or overloaded reviewers.

How to Assign Code Reviewers Without a Bottleneck

Othman Shareef — Wed, 02 Sep 2026 08:00:00 +0000

Ask a team how to assign code reviewers and you will usually get a shrug: “whoever knows that code.” It sounds reasonable and it is how, eighteen months later, one engineer is the reviewer for half the codebase, merges wait on their calendar, and nobody else can safely touch the payment path. Reviewer assignment is a real design decision with three basic strategies, one seductive trap, and a knowledge-transfer lever most teams never use on purpose.

The short answer: Assign code reviewers with a default and an override: round robin as the default so load and knowledge spread, expertise as the deliberate override for high-risk changes. Never let one expert become the standing reviewer for a whole area; that trades short-term safety for a bottleneck and a bus factor. One reviewer is enough for most changes.

How to assign code reviewers: the three models

Every assignment scheme is one of three ideas, or a blend:

Round robin : reviews rotate through the team regardless of familiarity. Even load, even knowledge spread, zero routing thought. Cost: reviewers regularly land on code they have never seen.
Expertise-based : the person who knows the area reviews it. Highest-quality feedback per review. Cost: expertise concentrates, and the expert becomes a queue.
Load-based : route to whoever has the fewest open reviews. Best turnaround times. Cost: optimizes for speed while ignoring both context and learning.

None of these is the answer alone. The teams that do this well run round robin as the default, override to expertise for changes with real blast radius, and use load only as a tiebreaker. The override list should be short and explicit (auth, payments, migrations, public APIs), which is the same tiering logic that belongs in a CODEOWNERS file.

You do not need custom tooling for the default, either. GitHub’s team review assignment can rotate requests through a team automatically, with round robin and load-balancing flavors built in, and that is a perfectly good baseline. What no platform automates is the override judgment: knowing which changes are risky enough to leave the rotation and go to a domain owner. That list lives in your head or in a document, and writing it down is most of the work of a real assignment policy.

The expertise trap

Routing everything to the expert is the trap because every individual decision is correct. For any single PR, the domain expert will give the best review, so each routing choice is locally optimal, and the sum is a disaster: a single human gate in front of an entire subsystem. The failure arrives on three fronts at once. Throughput: their review queue sets the team’s merge rate. Resilience: their vacation is a freeze, their departure is a crisis, and the bus factor of the whole area is one. Quality: review volume grows until skimming is the only way to keep up, at which point you have the bottleneck and shallow reviews.

The uncomfortable part is that experts rarely fight this arrangement. Being the mandatory reviewer is status, and handing off reviews feels like risk. The team lead has to break the loop deliberately, because it does not break itself; it only deepens.

Assignment is how knowledge actually spreads

The research is consistent that review is not mainly a defect filter. Microsoft’s study of practice at scale found knowledge transfer among the top outcomes reviewers and authors actually value (Bacchelli and Bird), and Google’s Critique case study lists education as an explicit goal of its review process, with most changes reviewed by a single reviewer. We covered the defect-finding evidence in do code reviews find bugs; the practical consequence for assignment is that every reviewer choice is also a teaching choice. Assigning the newcomer to review the subsystem they will inherit next quarter is not a diversity gesture; it is the cheapest onboarding the team has. Pairing them with a domain owner as second reviewer for the first few rounds, as in our first code review guide, converts review time directly into bus-factor insurance.

Self-selection cultures

Some teams skip assignment entirely: PRs land in a shared queue and reviewers pick what they take. At its best this is the fastest model, because motivated reviewers grab work they care about the moment it appears. It fails in two predictable ways. Unglamorous PRs (dependency bumps, test refactors, docs) age in the queue while interesting ones are fought over, and quiet team members review far more than loud ones. Self-selection works when paired with a backstop: anything unclaimed after a few hours gets assigned by rotation, and the queue is visible enough that aging PRs embarrass the team into action. Without the backstop, self-selection is just unassigned work with better branding.

Route by who can unblock what

Whatever model you choose, the question that should drive the day-to-day is not “whose turn is it” but “which review, done next, unblocks the most.” A release-blocking fix waiting on review outranks a refactor, whoever the assigned reviewer is; a teammate stuck until their PR lands outranks one who has moved on to other work. Most tools make this invisible, which is why we built a team view in ours (Pyor) that ranks waiting reviews by unblock value rather than by age. Tooling aside, the principle stands on its own, and it pairs with honest capacity planning: assignment decides who reviews, but priority decides what gets reviewed first, and teams that only manage the first half still ship late.

Frequently asked questions

Is round robin a good way to assign code reviewers?

Round robin is the best default and the worst final answer. It spreads load and knowledge evenly and kills the everything-expert bottleneck, but it routinely assigns reviewers with no context for the change. Use it as the baseline, then override deliberately: high-risk changes go to domain owners, everything else stays in the rotation.

Should the most senior engineer review everything?

No. It feels safe and creates two failure modes at once: a bottleneck, because every merge now waits on one calendar, and a bus factor, because context concentrates in one head. Volume also degrades their review quality into skimming. Reserve senior attention for the riskiest changes and let the rotation handle the rest.

How many reviewers should a pull request have?

Usually one. Google’s study of its internal review practice found most changes are reviewed by a single reviewer, and that lightweight convention keeps velocity high without measurable quality loss. Add a second reviewer deliberately for high blast-radius changes or when the goal is teaching, not for routine work, where extra reviewers mostly diffuse responsibility.

CODEOWNERS Best Practices That Keep Reviews Honest

Othman Shareef — Mon, 31 Aug 2026 08:00:00 +0000

CODEOWNERS is one of those GitHub features that teams set up once, half correctly, and never look at again until a release is blocked by a review request to someone who left the company in March. Most CODEOWNERS best practices are not clever tricks; they are consequences of how the file actually behaves, which is documented in GitHub’s code owners docs and routinely misremembered. Worth getting the mechanics right first, then the judgment calls.

The short answer: Treat CODEOWNERS as a routing table for review attention, not an org chart. Own directories rather than files, prefer teams to individuals, never let one person own everything, and audit quarterly for owners who no longer review. And remember the one rule that breaks most setups: patterns are evaluated top to bottom and the last match wins.

CODEOWNERS best practices start with the mechanics

The behaviors that matter, straight from the docs. GitHub searches for the file in .github/, the repository root, then docs/, and uses the first one it finds. Owners are automatically requested for review when a PR modifies code they own, though draft PRs wait until they are marked ready. Teams can be owners via @org/team-name, but the team must be visible and hold write permission on the repo, even if all its members already have write access directly.

Two sharp edges deserve special respect. First, the syntax looks like gitignore but is not: negation with ! and character ranges with [] do not work. Second, invalid lines are silently skipped, so a typo does not fail loudly; it just quietly stops routing reviews. If your CODEOWNERS file has drifted, nothing tells you.

Last match wins, so order your file like it matters

The docs say it plainly: order is important, and the last matching pattern takes precedence. That is the opposite of the first-match instinct people carry over from firewall rules and route tables, and it is the single most common CODEOWNERS bug. The working structure is broad-to-specific:

A default owner at the very top (* @org/eng), if you want one at all.
Directory-level ownership in the middle.
High-risk overrides at the bottom (/payments/ @org/payments), where nothing can shadow them.

Write it in the other order and your carefully chosen payments owners are silently overridden by the catch-all. Because lines fail silently, the only way you find out is by noticing the wrong people getting review requests.

Own directories, not files

File-level ownership rots at the speed of refactoring: files get renamed, split, and moved, and each move silently orphans a rule. Directory-level ownership tracks the actual shape of responsibility (a service, a package, a surface) and survives file churn within it. If you find yourself needing file-level rules, that is usually a signal the directory mixes concerns that want to be separated anyway. The same logic that makes directories the right unit for config and infra review makes them the right unit for ownership: the boundary is architectural, not alphabetical.

Avoid the everything-owner

The most tempting line in any CODEOWNERS file is * @that-one-senior-dev. It feels safe: every change gets an experienced pair of eyes. What it actually creates is a single human queue in front of every merge. That person’s vacation becomes a release freeze, their busy week becomes everyone’s slow week, and the volume guarantees the reviews become skims. When required review from code owners is enabled, an approval from any listed owner satisfies the requirement, so the honest fix is listing a team: the requirement stays strong while the load spreads. An everything-owner is a bus factor of one, enforced by branch protection.

CODEOWNERS as tier encoding

The most useful mental model: CODEOWNERS is where you encode review tiers by blast radius so the platform enforces them. Auth, payments, migrations, and public API directories get named owning teams plus required code-owner review in branch protection. Ordinary application code gets team-level ownership with no required gate. Docs and internal tooling may get no entry at all. The file becomes a readable statement of what the team considers dangerous, and the expensive reviewers are automatically routed to exactly the changes that warrant them, with no lost review requests along the way.

Keep it honest: audit for stale owners

A CODEOWNERS file is a claim that certain people will review certain changes. Claims go stale. The owner who changed teams, the person who left, the team that was reorganized away: each becomes either a review request into the void or, worse, a required approval nobody can give. A quarterly audit is cheap and mostly mechanical: list every owner, check they still exist and still have write access, then check the last few PRs in their area and confirm they actually reviewed. An owner who has not reviewed anything in their directory for two quarters is not an owner; they are a label. Either they recommit, or the line changes to the team that has actually been doing the reviewing. The file should describe reality, because branch protection will enforce whatever it says, real or not.

One last habit that keeps all of this working: treat changes to CODEOWNERS itself as reviewable code, not admin housekeeping. A one-line edit can silently reroute every review in a subsystem or shadow a high-risk override, and because invalid lines are skipped rather than rejected, a typo ships as easily as a decision. Give the file its own ownership entry so edits to it always get a deliberate second pair of eyes.

Frequently asked questions

Where does the CODEOWNERS file go?

GitHub looks for CODEOWNERS in the .github/ directory, the repository root, or docs/, in that order, and uses the first file it finds. The file is read per branch, so different branches can define different owners. Most teams use .github/CODEOWNERS to keep the repo root uncluttered and the file next to other GitHub configuration.

How does CODEOWNERS decide which owner applies when patterns overlap?

Last match wins. GitHub evaluates the file top to bottom and the last pattern that matches a changed file takes precedence, the opposite instinct of firewall-style first-match rules. Put broad patterns like a default owner at the top and specific overrides like /billing/ below them, or the specific rules will never fire.

Can a GitHub team be a code owner?

Yes, using the @org/team-name form, and it is usually the right choice: individuals go on vacation and leave companies. The team must be visible and must have explicit write permission on the repository, even when every member already has write access individually. When review from code owners is required, one approval from any listed owner satisfies it.

Code Review SLAs That Teams Actually Keep

Othman Shareef — Sat, 29 Aug 2026 08:00:00 +0000

Most teams that adopt a code review SLA do it after the same painful week: a two-line fix sat unreviewed for four days, someone shipped a hotfix around the process, and a retro produced the sentence “we need review SLAs.” Then the SLA gets written as “all PRs reviewed within 24 hours,” nobody instruments it, and three months later it is a dead rule everyone politely ignores. The failure is not the idea. It is that the SLA promised the wrong thing.

The short answer: A code review SLA that survives contact with a real team promises responsiveness, not completion: a first response within hours and a full review within one business day. Pair it with scheduled review blocks so keeping the promise costs no focus, and an escalation path that treats a breach as a routing problem rather than a personal failing.

Promise a first response, not a finished review

The distinction that separates SLAs teams keep from SLAs teams abandon: what exactly is promised. A finished-review SLA makes the reviewer accountable for the size and clarity of a diff they did not write. When a 2,000-line PR lands, the reviewer facing a 24-hour completion clock has two options, and both are bad: skim and approve, or blow the SLA and stop believing in it.

A first-response SLA promises something the reviewer actually controls: within a few working hours, the author hears one of “reviewing now,” “I can get to this at 3pm,” or “I am the wrong person, try Dana.” Any of those beats silence, because silence is what actually burns authors. Lou Franco made the sharp observation that authors control much of their own wait time: small PRs that take under fifteen minutes to review get picked up as mini-breaks between tasks, while big ones wait for a mythical free afternoon. The SLA covers the reviewer’s half of that bargain; the author’s half is keeping the diff small enough that a fast response is even possible.

One business day is the ceiling, not the target

Google’s engineering practices are unambiguous on the outer limit: one business day is the maximum time to respond to a review request, and the guidance frames speed as a first-class property of the whole system, because a slow review blocks the author, the feature, and everyone waiting behind the merge. Note what the rule is: a ceiling. A team whose median review lands in two hours and whose worst case is one business day has a healthy distribution. A team whose median is one business day has normalized the worst case.

This is also why a code review SLA should be measured at the distribution level, not policed per PR. Track time-to-first-response weekly, look at the tail, and ask what the slowest 10% have in common. Usually the answer is not a lazy reviewer. It is a lost review request: a notification buried in email, a re-request nobody saw, a PR assigned to someone on vacation. Fix the routing and the tail collapses on its own.

Review blocks make a code review SLA cheap to keep

The standard objection to any code review SLA is interruption cost: “I cannot drop into someone’s PR every time a notification fires and still do deep work.” The objection is correct, and the answer is not to soften the SLA but to change when reviews happen. Two or three scheduled review blocks per day (start of day, after lunch, before close) let a reviewer meet a four-hour first-response promise without ever being interrupted mid-task.

Predictability is the real product here. An author who knows reviews happen at 9, 1, and 4 stops pinging, plans their day around those slots, and queues work accordingly. The team stops paying the coordination tax of “is anyone looking at this?” This is the same capacity argument we made in review capacity planning: review time is a budgeted resource, and a budget you schedule is one you actually spend. An SLA without scheduled capacity behind it is a wish.

Scale the clock to the diff

A flat SLA treats a one-line config change and a cross-cutting refactor as the same object, which quietly rewards authors who batch everything into one giant PR (one SLA clock instead of five). Better: keep the first-response promise flat, and let the full-review expectation scale with size and risk. A small, well-described PR should get a complete review in the same block where it got its first response. A large one gets an honest timeline in that first response instead of a fake deadline. If large PRs dominate the queue, the SLA is not your problem; PR size is.

When the SLA breaks: escalate without shame

Every SLA breaks. The design question is what happens next, and the answer determines whether people keep reporting honestly. The escalation path that works is mechanical and blame-free:

At the deadline, the author sends exactly one direct nudge.
Two hours later, the PR moves: a named backup reviewer or the team’s shared review queue picks it up. The original reviewer is off the hook, no explanation required.
Repeated breaches surface in the weekly numbers as a capacity conversation (“reviews are landing on two people”) rather than a performance one.

The moment a missed SLA becomes an accusation, reviewers start gaming it: placeholder “looking!” comments that buy time, quick approvals that end the clock. An SLA is there to make waiting predictable and to surface routing and capacity problems early. Keep it aimed at the system and teams keep it; aim it at individuals and it dies in a quarter.

Frequently asked questions

What is a reasonable code review SLA?

First response within four working hours, full review within one business day. Google’s engineering practices treat one business day as the outer limit, not the target. The first-response promise matters more than the completion promise: an author who hears “looking at this after lunch” can plan around the wait, while silence forces them to guess.

Should a code review SLA require the review to be finished?

No. Promise responsiveness, not completion. A finished-review SLA punishes reviewers for large or messy PRs they did not write, so they either rubber-stamp to hit the clock or quietly ignore the rule. A responsiveness SLA (acknowledge, give a timeline, or hand off) is within the reviewer’s control regardless of diff size, so it survives.

What should happen when a review misses its SLA?

Escalation should reroute the work, not shame the person. A missed SLA usually means the reviewer is overloaded or away, so the fix is a visible next step: the author pings once, then the PR moves to a backup reviewer or a team queue. Treating breaches as routing signals keeps people honest about capacity instead of hiding behind silence.

How to Do Your First Code Review

Othman Shareef — Thu, 27 Aug 2026 08:00:00 +0000

Someone added you as a reviewer, and your first instinct is probably that they made a mistake: you barely know this codebase, and the author has been here for years. This guide to how to do your first code review starts from a different premise. You were asked because your read matters, not because anyone expects you to know everything, and the habits you build in the first few reviews decide whether review becomes a skill or a performance.

The short answer: Your first code review is a reading exercise, not an exam. Pick a small PR, read the description before the diff, follow the logic, and ask genuine questions wherever it loses you. Say plainly what you checked and what you did not. An approval scoped that honestly is worth more than a confident rubber stamp, and every review teaches you the codebase faster than writing code does.

You were asked because your read matters

The author has been staring at this change for hours or days, and they physically cannot see it fresh anymore. You can. That is the asset a new reviewer brings, and it does not require seniority. If the change does not make sense to you, that is data about the code, not about you: code that confuses a careful reader today will confuse the on-call engineer at 3am next year. Your job is not to certify perfection. It is to report, accurately, what the change did to one attentive reader.

How to do your first code review: start small

Do not make your debut on the 900-line feature branch. Pick small PRs deliberately, because size is the strongest predictor of whether a review is real: a 40-line bug fix you understand end to end teaches you more than an 800-line diff you skim. There is a social bonus, too. Lou Franco’s writeup on getting reviewed faster observes that small PRs get picked up quickly because reviewers grab them as mini-breaks between tasks. Be that reviewer. Small, prompt reviews make you the teammate whose name people are glad to see in the reviewer box, and they build your calibration one digestible change at a time.

Read the description before the diff

Know what the change claims to do before you check whether it does it. The description, the linked ticket, and the test names tell you the intent; the diff only tells you the implementation, and you cannot judge an implementation against an intent you never read. If the description is empty, that is legitimately your first comment, not an obstacle to route around. And if you can run the change locally, do: five minutes of clicking through the actual behavior grounds everything else you will say.

Ask questions as questions

Microsoft’s research on modern code review found that understanding the change is the top challenge reviewers face, seniors included. So “why does this need a lock?” is a contribution, not an admission: either there is a reason, which now gets written down where the next reader will find it, or there is not, and you just caught something. Ask genuine questions plainly, without dressing them up as verdicts or dressing verdicts down as questions. The author answering “why” in a thread is documentation being written in real time.

Lean on a checklist, not on nerve

You do not yet hold the team’s review standards in your head, and you do not need to. A code review checklist removes the blank-page problem: does the error path do something sensible, do the tests assert behavior rather than implementation, do the names say what things do, does anything touch data it should not. Walking a checklist beats staring at a diff waiting for insight, and after a dozen reviews the checklist moves into your head where it belongs.

Know what your approval means

Approving is not politeness; it says “I read this, and I am willing for it to merge on my word.” The empty thumbs-up that skips the reading part is review theatre, and it is the one habit that will quietly zero out your value as a reviewer. The honest alternative is scoping: “I reviewed the logic and the tests; someone else should check the infra part” is a perfectly good review, and saying it out loud is a sign of care, not weakness. Do that consistently and something compounding happens: every change you trace, every “why” you get answered, every checklist pass builds your map of the system. Reviewing teaches you a codebase faster than writing code in it does, because you see every part of it change, with the reasons attached. Your first review is the slowest one you will ever do. Your tenth will feel routine, and by then the team will have learned what your approval is worth.

Frequently asked questions

What if I do not feel qualified to review the code?

You were asked for your read, not your omniscience. A reviewer who carefully follows the logic and says where it loses them adds value even with less context than the author. Scope your approval honestly: say what you checked and what you did not, and ask someone else to cover the parts outside your depth. That is professionalism, not weakness.

Is it okay to just ask questions in a code review?

Yes, and it is more useful than it feels. Research on code review at Microsoft found that understanding the change is the hardest part of review for everyone, not just newcomers. A question that makes the author explain a decision often surfaces the bug, and the written answer documents the code for the next reader. Questions are contributions.

How long should my first code reviews take?

Shorter than you think. Pick small pull requests, read the description first, and give the change one careful pass instead of three anxious ones. Reviewer attention degrades after about an hour of continuous reading, so timebox yourself. A focused thirty minutes on a small PR beats an afternoon of second-guessing on a large one you were not ready for.

When to Request Changes in a Code Review

Othman Shareef — Tue, 25 Aug 2026 08:00:00 +0000

Every pull request review ends in one of three verdicts, and most reviewers pick between them by mood. Knowing when to request changes in a code review, versus approving with your comments attached, is one of the highest-leverage judgment calls a reviewer makes, because the verdict, not the comments, decides whether the author’s day continues or stalls. The comments carry the information. The verdict carries the cost.

The short answer: Request changes only when merging would cause harm you can name: a correctness bug, a security hole, data loss, or a design decision that becomes expensive to reverse once code depends on it. Everything opinion-shaped goes into comments on an approval, trusting the author to handle them. Wrongly blocking costs trust and a day of velocity; wrongly approving usually costs a follow-up PR. Price the verdicts accordingly.

Three verdicts, three signals

Approve: nothing further needed from me. Merge when ready.
Approve with comments: I found things worth saying, and I trust you to handle them without another round trip.
Request changes: merging now would cause concrete harm, and I need to look again before this ships.

These are messages about trust and process, not just quality scores. Google’s review guidance treats review latency as a first-order cost: respond within one business day at the outside, because slow reviews drain velocity and morale together. A request-changes verdict schedules at least one more full latency cycle. That is sometimes exactly right. It should never be an accident.

When to request changes in a code review

The rule that survives contact with real teams: block on correctness, security, or irreversibility, and nothing else. If you cannot finish the sentence “if this merges, X breaks,” “X leaks,” or “X becomes very hard to undo,” you do not have a blocking concern. You have a comment. Irreversibility deserves the emphasis: a badly named local variable is a thirty-second rename next week, while a badly designed public API or database schema accretes dependents the moment it lands. Block on the second kind, comment on the first, and be honest with yourself about which one you are looking at. A useful self-check: if the author shipped it exactly as written and you were on call, would you actually be paged, or merely annoyed? Paged is a block. Annoyed is a comment, however strongly you feel it.

Approve with comments is the default

Everything opinion-shaped lands here: naming, structure you would have chosen differently, style beyond what the linter enforces, the refactor that could ride along but does not have to. Marking severity explicitly makes this verdict work: Conventional Comments prefixes like nit: and suggestion: tell the author what is optional, so an approval with six notes reads as help rather than as a passive-aggressive block. The verdict is also a deal: you extend trust that the author will read and act in good faith, and authors who repeatedly merge without reading the notes are the reason the deal has to stay explicit.

The asymmetric cost of the wrong verdict

Wrongly approving usually costs a bug, a revert, or a follow-up PR: real but bounded, and mostly paid by the code. Wrongly blocking is paid by the person. It costs a latency cycle, teaches the author to route future PRs around you, and over time trains a team to fear review instead of using it. Both failure modes are real, and the asymmetry is not an argument for rubber-stamping: it is an argument for spending your blocks where they are unambiguous. A reviewer who blocks rarely, and only with a nameable harm attached, finds that nobody argues when they do.

Re-review the delta, not the PR

Etiquette after you request changes: when the author pushes fixes, review what changed since your last pass, not the whole PR from scratch. Confirm the blocking issues are resolved, resist discovering brand-new nitpicks on lines you already read, and answer with the same day-one latency you would want yourself. Nothing corrodes a review culture faster than a re-review that arrives three days late and raises objections that were visible in round one.

Say what you reviewed

Every verdict gets sharper with a scope statement: “reviewed the logic and the migration, skimmed the tests, did not look at the Terraform.” It makes an approval honest, tells the author what still needs eyes, and matters most on large PRs where nobody actually reviews everything. The verdict says whether the change can merge. The scope statement says what that verdict is worth. Reviewers who provide both are the ones whose approvals mean something, and whose rare blocks get taken seriously without a fight.

Frequently asked questions

When should I request changes on a pull request?

Block only when merging would cause harm you can name: a correctness bug, a security hole, data loss, or a design decision that will be expensive to reverse once other code depends on it. Everything else, naming, style, structure you would have done differently, belongs in comments on an approval. If you cannot name the concrete harm, do not block.

What does approve with comments actually mean?

It means: I trust you to address these notes without me re-checking. The author fixes what they agree with, replies where they differ, and merges without another round trip. It is the right default for opinion-shaped feedback because it delivers the signal without spending a day of latency on it. Reserve plain approval for changes with genuinely nothing to say.

How should re-review work after I requested changes?

Review the delta, not the whole PR again. Look at what changed since your last pass, confirm the blocking issues are resolved, and resist raising brand-new nitpicks on lines you already read once. Re-review should be fast by design: if the author addressed the blockers, dragging out extra rounds punishes them for responding to feedback.

Code Review Comments: Examples That Actually Land

Othman Shareef — Sun, 23 Aug 2026 08:00:00 +0000

Search for code review comments examples and most of what you find is etiquette: be kind, be constructive, assume good intent. All true, and all useless at the keyboard, because kindness is not a technique. What actually separates a comment that lands from one that starts a fight is craft you can practice: what the comment points at, whether it carries a reason, and whether the author can tell how much you mean it.

The short answer: A review comment lands when three things are true: it points at the code rather than the person, it carries the reason and not just the instruction, and its severity is explicit so a nitpick cannot impersonate a blocker. Add genuine questions when you do not understand intent, and specific praise when you see a pattern worth spreading, and most tone problems disappear without anyone trying to be nicer.

Point at the code, not the coder

The word “you” is the most dangerous word in a review. “You forgot to handle the timeout” and “this path swallows the timeout” describe the same line, but the first keeps score and the second reports a fact. Facts invite correction; verdicts invite defense. This is not about sparing feelings so much as keeping the discussion attached to the diff, where it can be resolved by editing code instead of by winning an argument. If a comment would read differently with the author’s name removed, rewrite it until it would not.

Give the why, not just the instruction

“Use a Set here” forces the author to either obey without learning or push back without information. “A Set makes the dedupe linear and says intent; the array scan goes quadratic on large inputs” is the same request, and it teaches. The why also serves the third audience every thread has: people reading it later, deciding whether the advice applies to their case. An instruction without a reason expires with the thread. A reason compounds.

Say how much you mean it

Half of review friction is severity ambiguity: the author cannot tell whether your comment blocks the merge or is a passing thought. Conventional Comments fixes this with prefixes: issue: for problems that need addressing, question: for genuine uncertainty, nit: for optional polish, praise: for patterns worth keeping. The labels cost nothing and make every comment self-triaging. They also keep you honest: typing nit: in front of a comment is a small forcing function to admit it is one, which connects to the larger argument that nitpicks should be automated or marked optional rather than delivered as ambiguous demands.

Code review comments examples: before and after

Before: “Why would you do it this way?” After: “question: is the retry loop intentional for the non-idempotent case? If this POST can fire twice, we may need an idempotency key.”
Before: “Wrong. Breaks on empty input.” After: “issue: parseCsv throws on a zero-length file because the header split fails. An early return with an empty result would match how loadJson handles it.”
Before: “Rename this.” After: “nit: fetchUser also writes the cache; loadAndCacheUser would say what it does. Fine to skip.”
Before: “This is not how we do error handling.” After: “issue: this catch block drops the original error, so the alert will fire with no cause attached. Our wrap-and-rethrow pattern is in errors.md; happy to pair on it.”

Every rewrite is longer. That is the cost: a sentence of reason and a severity label. It is cheap compared to a deadlocked thread or a colleague who quietly stops asking you for review.

Praise that teaches

Praise in review is not decoration, and it is not the empty approval that makes LGTM culture so corrosive. Specific praise is training data for the team: “praise: making the cursor opaque here is exactly right, it stops clients from depending on the encoding” tells the author what to do again and tells every other reader what good looks like in this codebase. Generic praise evaporates. Specific praise replicates the pattern.

When a thread deadlocks, leave the thread

Microsoft’s research on modern code review found that understanding the change is the top challenge reviewers face, which explains why long threads so often talk past each other: the participants are working from different models of what the code is doing. Three rounds of written back and forth is the practical limit. After that, take it to a call or a whiteboard, converge in minutes, then come back and record the outcome as one final comment. The record is the part people skip and the part that matters: “discussed offline, keeping the queue, adding a lag metric” turns a private resolution into context the next reader inherits for free.

Frequently asked questions

How do I write code review comments that do not offend?

Comment on the code, not the coder. "This connection never closes" lands; "you forgot to close the connection" keeps score. Give the reason with the request, mark severity honestly so a nitpick cannot masquerade as a blocker, and ask a genuine question when you are unsure of intent. Most defensiveness in review is a reaction to verdicts delivered without reasons.

What are Conventional Comments?

A lightweight convention for prefixing review comments with a label such as issue:, question:, nit:, or praise:, so severity travels with the words. The author can tell at a glance what blocks the merge and what is optional polish. It costs nothing to adopt, works in any tool, and removes the most common failure in review threads: mistaking a suggestion for a demand.

What should I do when a review comment thread deadlocks?

Leave the thread. After roughly three rounds of back and forth, written debate stops converging and starts hardening positions. Take it to a call or a whiteboard, decide together, then return and record the outcome in one final comment so the resolution is visible to future readers. A sentence like "discussed offline: keeping the queue, adding a lag metric" preserves the context.

How to Review Infrastructure as Code Changes

Othman Shareef — Fri, 21 Aug 2026 08:00:00 +0000

Every team has the story: a three-line YAML change sails through review with a thumbs-up in ninety seconds, and twenty minutes after deploy, production is down. Nobody was careless by their own standards. The reviewer applied the calibration that works for application code, where small diffs are usually safe diffs. To review infrastructure as code well, you have to unlearn that instinct, because in config the two are unrelated, and often inverted.

The short answer: Config and infrastructure diffs under-signal: they look tiny while touching everything. Review them by asking four questions the diff does not answer. Which environment does this actually hit? What defaults change silently underneath it? What permissions or secrets widen? How does it roll back? And insist on plan output as the review artifact, because the diff shows your edit while the plan shows what the platform will do.

Why config diffs under-signal

Application code advertises its complexity. A 400-line refactor looks scary and gets attention; a two-line change to a values file looks trivial and gets a glance. But config is dense with leverage: one value in a base template is inherited by every environment and every service that extends it. Diff size is a terrible proxy for risk, which is the whole argument of reviewing by blast radius, and infra is where the mismatch is worst. The smaller and more central the file, the more it usually touches. A reviewer who spends ten minutes on a three-line Terraform change is not being slow. They are being calibrated.

Which environment does this actually hit?

The first question, and the one most often skipped. Overlays, inheritance chains, workspaces, and templating all mean the file path lies about the scope: a change to base/ hits everything that inherits from it, and a file named staging.yaml can feed a module that production also consumes. Make the author say it in the PR description: which clusters, which accounts, which stages this lands in. The postmortems where a staging tweak turned out to be global almost always contain a reviewer who assumed the filename was the answer.

A checklist to review infrastructure as code

Environment scoping. Name every environment the change touches. If the author cannot enumerate them, that is the review finding.
Implicit defaults. A provider bump or chart upgrade can change defaults underneath a file that did not change at all. If versions moved, ask what defaults moved with them.
Secrets and permissions. Any widening of IAM roles, security groups, or service account scopes is a security review, not a config review. Treat a new * in a policy as a finding until justified.
Plan output in the PR. Require terraform plan, kubectl diff, or the equivalent as a PR artifact, generated by CI so it reflects real state.
Rollout and rollback story. How does this deploy, and does reverting the commit actually revert the change? Some infra changes are one-way doors the same way database migrations are: the revert is a second migration, not an undo.

The plan is the review artifact

The diff is what you wrote. The plan is what will happen, and they diverge constantly: state drift, module version bumps, and provider defaults all produce changes the diff never mentions. Reviewing a Terraform PR without plan output is reviewing a function by reading its name. The workable pattern is CI posting the plan into the PR on every push, so the reviewer reads intended edits and actual effects side by side. Read the plan with a simple priority: anything that destroys or replaces a resource first, permission changes second, everything else after. A plan that replaces a database to rename a tag is exactly the kind of thing the diff will never tell you.

AI wrote the YAML; who owns the why?

More and more infra is generated: an agent writes the Terraform, the human skims it, CI is green, merge. Osmani made the general argument that generation got cheap while understanding stayed expensive, and infra is where that gap bites hardest, because generated config looks authoritative while embedding defaults nobody chose. The instance size, the retention window, the ingress rule: were those requirements, or fill? The review has to distinguish the two, and the author has to capture the intent while it still exists. Six months from now, someone will stare at that retention value during an incident and need to know whether it was a decision or an accident. The PR is the only place that answer can live.

None of this makes config review slow. It makes it proportionate: ninety seconds was never the real cost of that three-line change, it was just the part paid before the incident.

Frequently asked questions

Why are small config changes so risky to review?

Because diff size and blast radius are unrelated in configuration. A three-line YAML change can retarget an environment, widen a permission, or change a default inherited by every service, while looking more boring than a 300-line refactor. Reviewers calibrated on application code give small diffs a glance, which is exactly backwards for infra.

Should terraform plan output be part of the pull request?

Yes. The diff shows what you edited; the plan shows what the platform will actually do, including changes pulled in by state drift, module updates, and provider defaults. Posting plan output in the PR, ideally generated by CI, turns review from guessing about effects into reading them. Destroy and replace lines deserve the most attention.

How should I review AI-generated Terraform or YAML?

Insist on the reasoning, not just the result. Generated infra tends to look plausible and complete while embedding defaults nobody chose deliberately. Ask which values were requirements and which the model invented, check permissions and network scope line by line, and record the intent in the PR while it still exists, because the prompt session will not survive.

How to Review Dependency Updates Without Reflex-Merging

Othman Shareef — Wed, 19 Aug 2026 08:00:00 +0000

Renovate and Dependabot turned dependency maintenance into a stream of small, identical-looking pull requests, and most teams responded by developing a merge reflex: green CI, click, next. The reflex is understandable, and for plenty of updates it is even correct. The problem is applying it uniformly, because that means you review dependency updates with the least attention exactly where breaking changes and supply-chain attacks concentrate. The fix is not reviewing harder across the board. It is deciding, before you open the PR at all, how much scrutiny this particular update has earned.

The short answer: Not all dependency updates deserve the same review. Tier them with two questions: how big is the version jump, and what can the package touch at runtime? A patch bump of a dev-only linter earns a reflex merge on green CI. A major of an auth library earns a changelog read, a lockfile inspection, and a look at every new transitive package entering your tree.

Why the merge reflex exists

Volume, mostly. A bot can open fifteen PRs on a Monday morning, and each one looks the same: two changed lines in a manifest and a wall of lockfile churn. SmartBear’s peer review research suggests keeping a review session under roughly 400 lines of code; a single lockfile regeneration can blow past that by an order of magnitude, so reviewers read none of it. When every update looks equally unreadable, every update gets the same three seconds. The reflex is not laziness. It is what happens when the process gives you no way to tell a boring update from a dangerous one.

Tier the update before you open it

Two questions sort almost every dependency PR: how big is the version jump, and how much do you trust what the package can reach?

Tier 0: patch or minor bumps of dev-only tooling (linters, formatters, test runners) in a repo with real CI. The blast radius is your build, not your users.
Tier 1: minor bumps of runtime dependencies. Skim the changelog, glance at the lockfile, merge.
Tier 2: major versions, anything in the auth, crypto, networking, or serialization path, and any update that runs install scripts or pulls new packages into the tree. This one is a real review.

This is the same reasoning as reviewing by blast radius generally: effort should follow consequences, not diff size. A patch bump of a formatter and a major of your OAuth client produce nearly identical diffs and have nothing else in common. Treat tier 2 updates of security-relevant libraries with the rigor of security-critical code, because that is what they are: code you are choosing to run with your users’ credentials in scope.

A checklist to review dependency updates

Read the changelog, not the version number. Release notes and migration guides say what actually changed; the semver bump only says what the maintainer believes changed. For majors, read the breaking-changes section before you read any diff.
Distinguish lockfile-only from manifest changes. A lockfile-only bump stays inside version ranges you already declared. A manifest change is a new contract. They look alike and mean different things.
Watch for new transitive packages. Every new name entering the tree is new supply-chain surface: a maintainer you now trust by default. A patch bump that adds six unfamiliar packages deserves more attention than a major that adds none.
Check for install scripts. A postinstall hook executes on every developer machine and CI runner. It is the classic delivery mechanism for a compromised package.
Note license changes. Rare, but a dependency relicensing from MIT to something restrictive is a legal change your project inherits silently.

Batch by tier, not by weekday

Batching is fine; blending is not. Group tier 0 updates into a weekly rollup so they cost one review instead of ten, but never let a tier 2 update ride into main inside a batch of fourteen boring ones. Give majors their own PR so the changelog reading has somewhere to happen, and so a revert removes one change instead of fifteen. Reading the risky ones is also partly a tooling problem: a raw lockfile wall hides the three lines that matter, and a review surface that groups the diff and separates manifest changes from lock churn makes the five minutes count. That is part of why we built Pyor (ours). But tiering beats tooling: even on plain GitHub, splitting the batch is most of the win.

When the reflex is the right call

Reflex-merging is genuinely fine for tier 0, provided the reflex is a policy rather than a mood. A patch bump of a well-tested dev dependency, in a repo whose CI actually exercises the affected path, does not need human eyes; it needs an automerge rule that names the packages and the version jumps it covers. The difference between a reflex and a policy is that a policy has edges: it says exactly which updates skip review, which means everything outside the edge gets one. The failure mode of dependency review is not merging fast. It is never deciding which updates deserve slowness, and letting fatigue decide for you.

Frequently asked questions

Should I review every Dependabot PR?

No, and pretending you will is how none of them get reviewed. Tier them instead. Patch and minor bumps of well-tested dev tooling can auto-merge on green CI. Runtime dependencies deserve a changelog skim. Majors, anything touching auth, crypto, networking, or serialization, and any update that adds new packages to your tree deserve a real review with the release notes open.

Are lockfile-only dependency updates safe to merge?

Safer than manifest changes, but not free. A lockfile-only update means your declared version ranges already allowed the new version, so no contract changed. The remaining risk is the supply chain: the new release itself could be compromised, and new transitive packages can enter the tree. Skim the lockfile diff for names you have never seen and for install scripts.

What makes a major version bump risky to reflex-merge?

Majors are where maintainers are allowed to break you on purpose. Behavior changes, removed APIs, changed defaults, and new peer dependencies all hide behind a version number that your CI may not exercise. Read the release notes and migration guide before the diff, and treat majors of security-relevant libraries as real code review, not routine maintenance.