DEV Community: Pyor

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.

How to Review a Refactor

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

“Pure refactor, no behavior change” is the most trusted sentence in code review and the least verified. Reviewers hear it, downshift into skim mode, and approve two thousand moved lines on vibes. Knowing how to review a refactor starts with reclassifying that sentence: it is not context, it is a claim, and claims need evidence. The author believes it, sincerely, every time. The production incident does not care about sincerity.

The short answer: “No behavior change” is a claim, not a property of the diff. Evidence looks like this: commits that separate mechanical moves from judgment rewrites, a test suite that passes without being edited, and tooling that verifies renames so you do not re-read them. When tests change inside a refactor, either the refactor is not pure or the tests were implementation-coupled; both facts belong in the review, in writing.

A refactor is a claim that needs evidence

The reviewer’s job in a refactor is not to admire the new structure; it is to verify the equivalence. That reframing changes what you ask for. Instead of “does the new code look right”, the questions become: what would show me the old and new code behave identically, which parts of this diff could a machine verify, and which parts require human judgment? A refactor PR that arrives as one monolithic commit with edited tests and a cheerful description offers no evidence at all, and the correct review response is not heroic reading. It is asking for the change to be restructured so that its safety is checkable.

How to review a refactor commit by commit

Commit structure is the single biggest lever, which is why atomic commits matter more in refactors than anywhere else. The reviewable shape is a sequence where each commit is one kind of change: a move-only commit, a rename-only commit, an extract-function commit, then the one commit that genuinely rewrites logic. Reviewed in order, the mechanical commits take seconds each and the judgment commit gets your full attention; reviewed flattened, the judgment change hides inside ten thousand lines of noise. This is where a review surface that scopes the diff per commit stops being a convenience and becomes the method (disclosure: commit-scoped diffs are a core feature of Pyor, ours, precisely because refactor review collapses without them). If the commits are not structured this way, the most valuable review comment is to ask for it: it costs the author an hour and saves the behavior.

Separate mechanical from judgment

Every refactor decomposes into two kinds of change, and they deserve opposite treatment:

Mechanical: renames, file moves, reordering, extract-with-identical-body. These are machine-checkable. Your job is not to read them but to confirm the tooling agrees they are what they claim to be.
Judgment: restructured conditionals, changed data flow, merged duplicate paths, new abstractions. These are where equivalence can silently fail, and they get the closest reading in the PR.

The ratio matters too. A refactor that is 95% mechanical and 5% judgment is a fast, safe review when the two are separated, and an unreviewable blob when they are mixed. Mixing them is how a flipped >= travels inside a moved block, invisible because the whole block is “just moved”.

The test suite is the harness, so it should not change

In a pure refactor, the strongest evidence available is boring: every existing test passes and the test files are untouched. The old assertions, written against the old code, now hold against the new code; that is as close to a behavioral proof as review gets. Which is why edited tests inside a refactor deserve immediate attention. Sometimes the edit is legitimate: the tests asserted implementation details (a private method name, an internal call order) that the refactor relocated. Fine, but then the description must say so, per test, because the alternative reading is that the behavior changed and the tests were updated to agree with it. Tests rewritten wholesale alongside the code they verify is the same failure we documented in the test-rewrite failure mode: nothing independent is left standing.

Renames and moves without re-reading everything

Nobody should re-read a thousand lines to confirm a file moved. Git already detects renames, and git diff --color-moved distinguishes blocks that moved intact from blocks that changed in flight, which is exactly the question a reviewer has. For refactors that land through force-pushed cleanups, git range-diff compares the old and new versions of the branch so re-review costs minutes instead of a second full pass. The principle underneath all the tooling: never spend human attention verifying something a machine can verify. Spend it where the machine is blind, on whether the new structure means the same thing as the old one.

The sneaked-in behavior change

The classic refactor failure is not a botched extraction; it is the “while I was here” fix. A null check added because it seemed obviously missing, a condition tightened, a default corrected: each one is a behavior change traveling under a no-behavior-change flag, unreviewed because the reviewer was told there was nothing to review. Some of those fixes are even right, which makes it worse: the wrong ones inherit the trust the right ones earned. The triage discipline from reviewing large PRs applies directly: sort the diff into what claims to be mechanical and what does not, verify the claim with tooling, and treat every line that fails verification as new, unreviewed logic. When you find a genuine fix hiding in a refactor, the answer is not to reject the fix. It is to ask for it as its own commit, with its own test, wearing its own name.

Frequently asked questions

How do I verify a refactor really has no behavior change?

Look for evidence rather than assurance: the test suite passes without any test being edited, mechanical changes (renames, moves) are isolated in their own commits where tooling can verify them, and the judgment changes are small enough to read closely. If tests changed, the refactor is either not pure or the tests were coupled to implementation details; either way the author owes an explanation in the description.

Should tests change in a refactoring PR?

In a pure refactor, no: unchanged, passing tests are the strongest evidence the behavior held. When tests must change, it is usually because they asserted implementation details the refactor just moved, and that is worth naming in review. The dangerous pattern is tests rewritten wholesale alongside the code, because then nothing independent verifies that the old behavior survived.

How do I review large renames or file moves without reading everything?

Make the tooling prove the mechanical parts so you only read the judgment parts. Git detects renames and, with move coloring, shows whether moved blocks changed in flight. Ask the author to isolate moves and renames in commits that contain nothing else; a move-only commit that tooling confirms is identical needs seconds, not an hour. Attention then goes to the commits that actually rewrote logic.

How to Review Test Code

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

Test code gets the worst review in the codebase. Reviewers spend their attention budget on the implementation, reach the test file with an empty tank, and scroll: it is long, it is repetitive, it is green in CI, approve. Yet the tests are the part of the PR that decides what the codebase is allowed to do forever after. If you want to know how to review test code well, the good news is that one question does most of the work, and it takes seconds to ask.

The short answer: One question does most of the work: if the behavior this test covers broke tonight, would the test fail? Tests that assert the implementation instead of the requirement pass right through real bugs. Review for that first, then for the edge cases that are missing, then for flakiness smells: sleeps, real networks, real clocks, shared state. And when an agent wrote the tests alongside the code, read them as claims, not as evidence.

Why tests get rubber-stamped

Partly fatigue: test files sit at the bottom of the diff, alphabetically and emotionally. Partly a category error: reviewers treat tests as ballast that proves the author did their job, rather than as code with its own failure modes. And partly LGTM culture: green checks feel like someone else already did the reviewing. But CI only proves the tests pass against this implementation. It proves nothing about whether they would fail against a broken one, and that second property is the entire point of having them.

How to review test code: the one question

For each test, run a small mental mutation: imagine the most plausible bug in the behavior under test (an off-by-one, a flipped condition, a dropped error path) and check whether any assertion would catch it. This takes seconds and it is brutal to weak tests. A test that asserts sendEmail was called once catches nothing about the recipient, the subject, or the body; the bug that emails the wrong user sails through. A test whose only assertion is that no exception was thrown certifies that the code ran, not that it worked. If you cannot think of a realistic bug the test would catch, say so in the review: that is not a nitpick, it is the finding.

Assert the requirement, not the implementation

The most common way tests go wrong is by encoding how the code works instead of what it must do. Mock-heavy tests that assert internal call sequences break on every refactor and catch no behavior change: they are change detectors, not correctness checks. Snapshot tests are the same failure at scale: they fail whenever anything changes, so updating them becomes reflexive, so they catch nothing. The review heuristic: read the assertion and ask whether it restates a sentence from the requirement. “Returns the three most recent orders” is a requirement. “Calls orderRepo.query with limit: 3” is an implementation detail wearing a test costume, and it will cost a future refactorer an afternoon.

Readability rides along here, because tests are the documentation people actually read. A reviewer should be able to tell, from the test name and the assertion alone, which requirement dies if the test dies. If you cannot, neither will the maintainer deciding in a year whether a failing test is a real regression or safe to delete.

Edge cases: review what is not there

The hardest part of reviewing tests is that the biggest defects are absences. Happy-path coverage looks complete because nothing marks the missing cases. Walk a short list against the behavior:

Empty and singular: zero items, one item, and the boundary the code branches on.
Absent and malformed: null, missing fields, the string where a number should be.
The error path: what the code does when its dependency fails. If the implementation has a catch block and no test exercises it, that block is unreviewed code.
The case from the ticket: the bug or requirement that motivated the PR should appear, recognizably, as a test.

Flakiness smells you can spot in the diff

Flaky tests are usually born flaky, and their birthmarks are visible in review. A sleep(500) standing in for synchronization is a race with a timer attached: it fails on the slow CI runner next month. Real network calls make the suite hostage to someone else’s uptime. Direct use of the real clock (Date.now, “expires tomorrow” fixtures) plants time bombs that go off on New Year’s Eve. Shared state between tests (module-level fixtures mutated by each case) makes the suite order-dependent, and the failure will reproduce only under the parallel runner, never on your machine. Each of these has a boring, standard fix (fake timers, fakes for I/O, injected clocks, per-test setup), and review is the cheap moment to insist on it.

Agent-written tests are part of the claim

Everything above doubles in importance when a model wrote the tests, because the same system produced the claim and the evidence. Osmani flags agents modifying tests until they pass as a defining failure mode of this era, and we have documented the test-rewrite failure mode in detail: deleted assertions, widened tolerances, and skipped cases are how a red suite becomes green without the code getting fixed. In an agent PR, read the test diff first, and treat any relaxation of an existing test as the highest-signal change in the whole PR. The broader checklist for reviewing AI-generated code applies, but the test-specific rule is simple: agent-written tests are part of the claim, not part of the evidence, until a human has asked the one question of each of them.

Frequently asked questions

What is the most important thing to check when reviewing tests?

Whether the test can fail for the right reason. Imagine the most likely bug in the behavior under test, then check that an assertion would catch it. Tests that assert a function was called, that a snapshot matches, or that no exception was thrown often pass straight through real regressions. A test that cannot fail when the requirement breaks is documentation at best and false confidence at worst.

How can I spot a flaky test during review?

Look for the ingredients rather than waiting for the flake: sleeps and fixed timeouts standing in for real synchronization, calls to real networks or real clocks, shared mutable state between tests, and assertions that depend on ordering nobody guarantees. Any of these in a diff predicts intermittent failure. Ask for fake timers, injected clocks, and explicit waits on conditions instead of durations.

Should AI-generated tests be reviewed differently?

Yes: with more suspicion, not less. When the same model writes the implementation and the tests, the tests tend to encode what the code does rather than what it should do, so they pass by construction. Review agent-written tests as part of the claim being made, not as independent evidence. Deleted assertions, widened tolerances, and skipped cases are the highest-signal lines in the diff.

How to Review Frontend Pull Requests

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

Frontend PRs collect the emptiest approvals in the codebase. A reviewer reads the JSX top to bottom, the markup looks plausible, the styles look plausible, and “looks fine” ships. The problem is that frontend correctness barely lives in the diff: it lives in what happens between renders, on slow networks, at odd viewport widths, and under a keyboard instead of a mouse. To review frontend pull requests well, review behavior. The diff is just the map, and the map is not the territory.

The short answer: “Looks fine” is not a frontend review. Every screen ships with at least four states (loading, error, empty, success) and the diff usually shows one. Review the states, the races between them, the keyboard path, and what the change does to the bundle. And when the change is visual or interactive, pull the branch and click around: ten minutes in a real browser beats an hour of imagining how the code behaves.

Review frontend pull requests in states, not screens

The single highest-value habit: for every piece of UI in the diff, walk the four states explicitly.

Loading: what renders while data is in flight? A layout that jumps when content arrives is a bug you can spot in the diff by the absence of a skeleton or reserved space.
Error: what does the user see when the request fails? “Nothing, the component just does not render” is the most common answer and the wrong one.
Empty: zero items is not an error and not a loading state. If the diff maps over a list with no empty branch, ask.
Success: the one state the author tested. Spend your remaining attention on the other three.

Race conditions hide between the states

State handling bugs rarely show up as wrong code; they show up as code that assumes events arrive in order. The classics are easy to spot once you name them. A typeahead fires a request per keystroke, and the response for “re” lands after the response for “react”, overwriting fresh results with stale ones: look for request cancellation or a staleness check. A submit button without a disabled-while-pending state will double-fire on a slow connection. An async callback updates state on a component that unmounted two navigations ago. When the diff adds any await between a user event and a state update, ask the ordering question: what happens if a second event fires before the first one resolves? If the answer requires optimism, it requires a fix.

Accessibility basics that take two minutes to check

You do not need an audit to catch the accessibility failures that matter most; you need a short list applied every time. Inputs have labels, not just placeholders. Interactive things are buttons or links, not a div with an onClick, because the div gets no keyboard or screen-reader behavior for free. Modals move focus in when they open and restore it when they close. Images that carry meaning have alt text; decorative ones have an empty alt. Text contrast survives the muted-gray-on-white aesthetic the design system keeps drifting toward. None of these require expertise to review, and catching them in the PR costs a comment; catching them after ship costs a ticket, a sprint, and an apology.

When to pull the branch and click around

Reading a diff answers “is this code reasonable”; only running it answers “does this work”. The threshold for pulling the branch should be low and explicit: any new user flow, anything animated, anything with focus or keyboard behavior, any layout change that must survive mobile widths. Once it is running, resize the window through the breakpoints, throttle the network in devtools and watch the loading states you just reviewed on paper, and tab through the new UI without touching the mouse. We have written before about reviewing locally versus in the browser; frontend changes are the strongest case for local, because the browser tab showing you the diff cannot show you the product. This is also where review tooling earns its keep: our own reviewer, Pyor, exists partly because switching between a fast diff surface and a running branch should not cost you your place in the review.

Bundle size and the dependency question

Every frontend PR that touches package.json is two reviews: the code and the dependency. The dependency review asks whether the platform already does this (surprisingly often: dates, formatting, clipboard, dialogs), whether the UI library already ships the component being rebuilt, what the new package weighs and whether it tree-shakes, and whether it is maintained by more than one tired person. Lockfile diffs measured in thousands of lines for a convenience helper deserve a conversation, not an approval. The reviewer is the last person positioned to ask, because after merge the dependency is load-bearing and the question is moot.

Screenshots and recordings are a review accelerant

The cheapest improvement to frontend review does not happen in review at all: it happens in the PR description. Before-and-after screenshots for visual changes, a short recording for interactions, viewport captures for responsive work. This is author self-review doing double duty: producing the capture forces the author through their own states, and the reviewer starts from observed behavior instead of reconstructing it from JSX. SmartBear’s review research found reviewers process only a few hundred lines per hour well; screenshots let you spend those lines on logic instead of mentally rendering markup. Teams that make captures a checklist item for UI changes review faster and argue less, because everyone is looking at the same pixels.

Frequently asked questions

What states should every frontend change handle?

At minimum four: loading, error, empty, and success. The diff usually shows success and nothing else. Reviewers should ask what renders while data is in flight, what the user sees when the request fails, and what an empty result looks like. Most frontend bugs that reach users are not broken success states; they are missing or wrong handling of the other three.

When should I run a frontend branch locally instead of reading the diff?

Whenever the change is interactive or visual: new flows, drag and drop, animations, layout changes, anything with focus or keyboard behavior. Reading JSX tells you what the tree renders; it cannot tell you how it feels, whether focus lands sensibly, or what happens on a slow connection. Ten minutes of clicking with throttled network catches what an hour of reading misses.

How should reviewers handle new frontend dependencies?

Treat every new dependency as a standing cost, not a one-time import. Ask three questions: does the platform or an already-installed library do this, what does it add to the bundle, and who maintains it? A date-formatting one-liner does not justify a library, and a component your UI kit already ships should never be rebuilt from a new package.

How to Review API Changes Without Breaking Clients

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

Most API diffs look harmless. A field added here, a validation tightened there, an error message reworded. The code is correct, the tests pass, and a week later a mobile release from three months ago starts crashing on a response it no longer understands. To review API changes well, you have to stop asking whether the code is right and start asking a different question entirely: who consumes this contract, and can any of them break? That is blast-radius review in its purest form, because the blast radius of an API change is every client you cannot redeploy.

The short answer: An API change is a contract change, and the reviewer’s job is to enumerate the parties to the contract. Sort every change into additive or breaking before reading any implementation. Treat serialization, nullability, and error-shape edits as breaking until proven otherwise. Require a deprecation path before any removal, and make spec, docs, and generated clients ride in the same PR. Correct code that breaks a client you forgot about is still a failed change.

Review API changes as contracts, not code

The implementation tells you what the server does now. The contract is what consumers were promised, and the promise includes everything they could observe: field names, types, nullability, ordering guarantees, status codes, error bodies, timeouts, and pagination behavior. The first review pass should ignore the handler logic and enumerate consumers instead: the web app, the mobile apps pinned to old releases, the partner integrations, the internal service three teams over, the cron job nobody remembers. For each one: can it break, and would anyone know before their users do? If the author cannot list the consumers, that is the review finding, and it outranks anything in the code.

Sort every change into additive or breaking

Before reading a line of implementation, classify the contract diff. Additive changes are safe by construction for well-behaved clients: a new endpoint, a new optional request field, a new response field that old clients will ignore. Breaking changes need a migration story:

Removing or renaming anything. A rename is a removal plus an addition; the removal is the part that breaks.
Changing a type. Including the sneaky ones: integer to string IDs, timestamps changing format, a scalar becoming an array.
Tightening validation. Requests that used to succeed now fail. Old clients cannot know the new rules.
Changing defaults or semantics. Same field, same type, different meaning: the schema validates and the behavior still breaks.

Breaking changes also deserve isolation. Following the logic of small, single-purpose changes, a breaking contract change buried inside a feature PR is the worst of both worlds: too easy to miss in review, too hard to revert alone. Ask for it as its own PR.

The quiet breakers: serialization, nullability, error shapes

The changes that survive review and still break clients are rarely the loud ones. They are the ones that keep the diff small and the schema technically valid. A field that was always present starts being omitted when empty, and a client doing response.items.length throws. A column becomes nullable in the database and the API dutifully starts returning null where a string always lived. A date serializer upgrade adds milliseconds to timestamps and a strict parser somewhere downstream rejects them. Error responses are the most neglected contract of all: clients parse error bodies to decide whether to retry, refresh a token, or show a message, so changing an error shape or swapping a 400 for a 422 is as breaking as renaming a field. When the diff touches a serializer, a nullable annotation, or an error handler, slow down: that is where the incident lives.

Versioning and deprecation are review items

When a change is genuinely breaking, the review question becomes: what is the path for existing clients? Acceptable answers are a new version (path, header, or field-level), or an explicit expand-and-contract migration: ship the new shape alongside the old, migrate consumers, then remove. Either way, removal needs a deprecation sequence, and every step of it is checkable in review: is the old behavior marked deprecated in the spec? Is there a sunset date? Is there telemetry counting calls to the old shape, so removal day is a data decision instead of a hope? A PR that deletes a field with no usage numbers attached should not pass review, no matter how confident the author is that nobody uses it. Somebody always uses it.

Generated clients, specs, and docs ride along

If the API has an OpenAPI spec, a GraphQL schema, or generated client libraries, those artifacts are part of the change and belong in the same PR. Two reasons. First, drift: a spec updated “in a follow-up” is a spec that lies for a sprint. Second, evidence: the regenerated client diff is the best breaking-change detector you have. You do not read generated code line by line; you scan it for deletions, because every deleted method or field in a generated client is a consumer-visible removal. Docs follow the same rule as tests: if behavior changed and the docs did not, the PR is incomplete. Make the contract artifacts a standing item on your review checklist so they stop depending on reviewer memory.

None of this makes API review slower once it becomes habit. It reorders it. Enumerate consumers, classify the change, hunt the quiet breakers, demand a deprecation path, check the artifacts. The handler logic, the part most reviews start and end with, comes last, because it is the only part of an API change the compiler and tests already have covered.

Frequently asked questions

How do I know if an API change is breaking?

Ask what an existing client, built against the old behavior and never updated, does when it hits the new one. Removing or renaming fields, changing types, tightening validation, and changing error shapes are breaking. Adding optional fields and new endpoints is usually additive. The trap is the middle: changes that keep the schema valid but change meaning, like a field that starts returning null.

Should generated client code be reviewed?

Yes, but differently: scan it, do not read it. The generated diff is evidence about the contract change. Deletions in a generated client mean something disappeared from the spec, which means some consumer loses a field or method. Review the generator inputs closely, then use the generated output as a breaking-change detector rather than reviewing it line by line.

What does a good API deprecation path look like?

Announce the deprecation in the spec and docs, keep the old behavior working alongside the new one for a stated window, emit telemetry so you know who still calls the old shape, and remove it only when usage reaches zero or the window expires. A review should reject removals that skip straight to deletion, because the missing step is always the telemetry.

How to Review Database Migrations

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

Database migrations are the changes most likely to hurt you and the hardest to undo, and they usually arrive as a five-line diff that gets approved in thirty seconds. To review database migrations well, stop reading them as code and start reading them as operations: something that will run exactly once, against production data, under production load, with no clean way back. That is blast-radius thinking applied to its most extreme case, because a bad migration does not break a feature. It breaks every feature that touches the table.

The short answer: Migrations deserve a different review posture because they combine the highest blast radius with the weakest rollback story. Read them as operations, not code: verify the down path actually restores something, know what locks the change takes on your biggest table, check that the old code survives the new schema during deploy, and insist that backfills ship as their own step. A migration review that skips any of these is a rubber stamp.

Why migrations justify breaking the PR-size rules

Normal review heuristics say right-size the PR and spend attention proportional to diff size. Migrations invert both. The schema change should ship alone, even if that means an extra PR that is three lines long, and those three lines deserve more scrutiny than the three hundred lines of application code that follow. A one-line ALTER TABLE can be the riskiest line your team merges all quarter. When a migration arrives bundled with feature code, the first review comment writes itself: split it out. Separate deploys are the whole point, because the schema and the code will never change at the same instant anyway.

The down path is part of the change

Ask what happens when this needs to be undone at 2am, because that is when it will need to be undone. Concretely:

Does a down migration exist at all? An empty down block or a raise-irreversible marker is sometimes honest and fine, but it must be a declared decision, not an omission.
Does it restore data or just shape? Re-adding a dropped column brings back the column, not the values. If the up path destroys data, the down path is a lie unless there is a backup step.
Has anyone actually run it? Down migrations are the least tested code in most repos. Ask for evidence: run up, run down, run up again on a real copy.

Lock behavior: ask what happens to the table while this runs

The silent killer in migration review is the lock you cannot see in the diff. The SQL says what changes; it does not say that the table is unreadable for four minutes while it happens. Questions to ask of every schema statement:

How big is this table in production? Row count changes everything. An instant operation on the dev database can be a full table rewrite at scale.
Does this statement rewrite or lock the table? Adding an index without CONCURRENTLY in Postgres, changing a column type, or adding a volatile default are classic full-lock traps. Know your engine: MySQL and Postgres disagree about which operations are online.
Is there a lock timeout? A migration waiting on a lock queues everyone behind it. Setting lock_timeout so the migration fails fast instead of stalling production is cheap insurance, and its absence is worth a review comment.

Deploy order: old code runs against the new schema

Code and schema never deploy in the same instant, so there is always a window where the previous version of the application runs against the migrated database. Most data-loss stories live in that window. Dropping a column the old code still reads throws errors until the deploy finishes. Renaming a column is worse: it is a drop and an add wearing a trench coat, and the old code writes into a column that no longer exists. The reviewable pattern is expand and contract: add the new column, dual-write, backfill, switch reads, and only then, releases later, drop the old one. When a migration renames or drops anything, the review question is not whether the SQL is correct. It is: which deployed code version reads or writes this, and what does it do during the window?

Backfills are a separate step, not a rider

A schema change that also moves data is two changes in one, and the second one is the slow, dangerous one. Google’s guidance on small changes applies with extra force here: the backfill should be its own step, reviewed on its own terms. What to look for when it is: batching (updating fifty million rows in one transaction will bloat and lock), idempotency (the job will crash midway at least once, so rerunning must be safe), throttling (the backfill competes with production traffic for I/O), and progress visibility (how does anyone know it is 40% done rather than hung?). A backfill inlined into the migration file fails most of these by construction, because migration runners assume fast, transactional, run-once semantics.

A checklist to review database migrations

Fold the above into a repeatable pass, the way a good checklist turns judgment into habit:

Reversible? If not, is irreversibility declared and accepted?
What locks does each statement take, on the production-sized table?
Lock timeout and statement timeout set?
Can the currently deployed code run against the new schema? The old schema?
Any dropped or renamed column still referenced by live code?
Backfill separated, batched, idempotent, throttled?
Was this tested against production-shaped data, and how long did it take?

The last item deserves emphasis because it is the cheapest to skip. A migration tested only on an empty development database has not been tested. Restore a scrubbed production copy, run the migration, and time it. The number you get is the difference between an invisible deploy and an incident review, and it is the single most persuasive line an author can put in a migration PR description.

Frequently asked questions

What makes database migrations risky to review?

Migrations run once, against production data, often while holding locks, and the worst mistakes are irreversible: a dropped column takes its data with it. The diff is usually tiny, so normal review instincts (small diff, quick approval) point the wrong way. Reviewing a migration means reviewing an operation: locks, deploy order, and the rollback story, not just the SQL.

Should a migration ship in the same pull request as the code that uses it?

Usually no. Schema and code deploy at different moments, so old code always runs against the new schema for a window. Shipping the migration separately forces everyone to think about that window explicitly, keeps the risky change reviewable on its own, and makes rollback simpler: you can revert code without fighting a half-applied schema change.

How do I test a migration against production-shaped data?

Restore a recent production backup (or a scrubbed copy) into a staging database and run the migration there, timing it and watching lock waits. Row counts are what matter: an ALTER that finishes instantly on a 200-row dev table can lock a 200-million-row production table for minutes. If a full copy is impractical, generate synthetic data at production scale for the affected tables.

Security Code Review Checklist for Critical Paths

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

Most review advice optimizes for throughput: smaller diffs, faster turnaround, fewer nits. Security-critical code is the place that advice goes to die. When a change touches authentication, payments, or anything that parses user input, the correct move is to slow down on purpose, and a security code review checklist is how you make that slowdown systematic instead of dependent on whoever happens to feel paranoid that day. Here is the checklist we run, and the policy that decides when it applies.

The short answer: Security-critical changes get reviewed slowly by policy, not by vibes. Route auth, payments, and input-handling diffs to your highest-rigor tier automatically, then run five checks: input validation at every trust boundary, authorization on every new path, secrets out of code and logs, injection surfaces wherever strings become queries, and error paths that do not leak. Two independent reviewers for the riskiest paths, no exceptions for small diffs.

Slow down by policy, not by vibes

The defining property of security review is that your adversary gets unlimited retries. A logic bug fires when a user happens to hit it; a security bug gets hunted by someone motivated, with tooling, forever. That asymmetry is why security-critical paths belong in the highest tier of the scheme we described in review by blast radius, and why the routing must be mechanical: path-based rules (an auth/ directory, a payments service, anything touching session handling) that flag the PR before any human decides how carefully to read it. Deciding rigor per-PR under deadline pressure always resolves the same way. Policy exists so the decision is already made.

The security code review checklist

This is deliberately short. It will not replace a security team or a scanner; it is what a reviewer holds in their head while reading a sensitive diff. The categories map onto the recurring failures in the OWASP Top Ten, which is worth reading in full at least once.

Input validation at trust boundaries. Find every point where data crosses from less-trusted to more-trusted: request bodies, headers, webhooks, file uploads, messages off a queue. Each crossing validates type, length, and range on arrival, not three layers deeper where someone assumes it already happened.
Authorization on every new path. Not authentication: authorization. The classic escape is a new code path to an existing resource that skips the ownership check the old path had. Ask of every new handler and every refactored branch: who can reach this, and did anyone verify they may?
Secrets handling. No credentials in code, config defaults, or fixtures. Watch logs especially: the fastest way to leak a token is to log the request that carried it.
Injection surfaces. Anywhere a string becomes a query, a shell command, a path, or markup. Concatenation is guilty until proven parameterized, including in the migration scripts and admin tooling nobody considers production.
Error paths that leak. What does the caller see on failure? Stack traces, internal hostnames, timing differences between “no such user” and “wrong password.” Failure output is API surface; review it like one.

AI-generated code inherits average security posture

A growing share of the diffs hitting these paths were written by models, and models reproduce the security posture of their training data: average, generic, and confidently incomplete. Generated code tends to include validation that looks right while missing the trust boundary your specific architecture actually has, because the model cannot know where your boundaries are. Osmani’s agentic code review essay frames the general problem: generation got cheap, verification stayed expensive, and volume pressures reviewers into exactly the skimming that sensitive paths cannot tolerate. The checklist from reviewing AI-generated code applies double here: no security assumption survives on the model’s authority, and “the tests pass” means little when the same model wrote the tests.

The two-reviewer rule for tier-3 paths

For the highest tier (auth flows, payment mutations, crypto, anything handling regulated data) one reviewer is not enough, and not because one person is careless. Single reviewers have single perspectives, and the parallel-review evidence says perspectives barely overlap: different readers flag almost entirely different issues. Two independent reviews of a sensitive diff are close to two distinct filters, not one filter run twice, and the second filter is cheap relative to the incident it prevents. Independence is the operative word: two approvals where the second reviewer skimmed the first one’s comments is one review with extra ceremony. Have both read cold, then compare. When their findings differ, that difference is itself information about where the diff is hard to reason about.

Make the slow path cheap to invoke

The failure mode of every rigorous process is that people route around it. If the security tier means a week of delay and an argument, engineers will quietly scope their changes to dodge the trigger paths, and you will have built an incentive to hide risk. Keep the heavy tier light everywhere you can: automate the routing, keep the checklist to the five items above, timebox the two reviews, and let everything outside the sensitive paths flow at normal speed. The goal is a team where flagging your own PR as security-relevant is a reflex, because doing so costs a day of extra scrutiny rather than a week of friction. Slow is a choice you make on purpose, in one place, so you can be fast everywhere else.

Frequently asked questions

What should a security code review checklist cover?

Five areas catch most of what matters: input validation at every trust boundary, authorization checks on every new code path (not just new endpoints), secrets kept out of code and logs, injection surfaces wherever strings become queries or commands, and error paths that leak internals. Run it on any change touching auth, payments, or user input, regardless of diff size.

How is reviewing security-critical code different from normal review?

The pace and the stance. Normal review optimizes for throughput; security review is deliberately slow by policy, because attackers get unlimited retries against whatever you approve. You read as an adversary looking for the path around the check, not as a colleague confirming the happy path works. Highest-risk paths also warrant two independent reviewers rather than one.

Is AI-generated code less secure than human-written code?

It reliably reflects the average security posture of its training data, which is not the posture your threat model needs. Models produce plausible validation and authorization code that misses your system’s specific trust boundaries, and the volume they generate pressures reviewers to skim. Treat AI-written changes on sensitive paths exactly like human ones: same checklist, same slow tier, same two reviewers.

A Code Review Checklist That Fits in Your Head

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

Most code review checklists fail the same way: they are written to be complete instead of usable. Forty items, three categories deep, checked by nobody after week two. A code review checklist earns its keep only if it fits in your head, because the whole point is to shape attention while you read, not to generate a compliance artifact afterward. Here is the version we actually use: three passes, seven questions, and clear rules about when to put the list down.

The short answer: A usable code review checklist is three ordered passes (correctness, design, style last) crossed with seven questions worth asking on every change: does it do what it says, what happens on failure, is it understandable, does it fit the design, what do the tests assert, what breaks downstream, and would you own it in production. Checklists build consistency and onboard reviewers; the moment they become box-ticking, throw the boxes away.

Three passes, in this order

Attention is the scarce input, so spend it in decreasing order of consequence. First pass: correctness. Does the logic do what the PR description claims, and what happens when inputs are hostile or dependencies fail? Second pass: design. Is this the right shape for the change, does it fit the surrounding system or fight it? Third pass, always last: style, and only what automation could not already flag. This is the same discipline we recommend in how to review large pull requests, and it matters because the reverse order is seductive: naming nits are easy to find and produce the feeling of review without its substance. The SmartBear peer review research caps effective review around 400 lines and an hour per session; passes force you to spend that budget on verdicts that matter before it runs out.

The code review checklist: seven questions

Does it do what the description says? Not approximately: trace at least one real path from entry to effect.
What happens on the unhappy path? Bad input, timeout, partial failure, double submit. Most escaped bugs live here.
Will someone understand this in six months? If you needed the author to explain it, the next reader will too, and they will not have the author.
Does it fit the existing design or fight it? A locally clean change that duplicates or bypasses an existing mechanism is a design bug.
What do the tests actually assert? Read the assertions, not the test count. Weakened or deleted assertions are the highest-signal lines in a diff.
What breaks downstream? Callers, consumers, migrations, API contracts: the blast lives outside the diff.
Would I own this line in production? If the pager went off tonight and this line was the cause, would you stand behind having approved it?

Seven is deliberate. It is small enough to run from memory on every review, which means it actually gets run. Notice what is absent: formatting, naming conventions, import order, anything a machine can check. A human checklist that duplicates the linter trains reviewers to do a robot’s job badly while the judgment questions go unasked. If an item on your current template could be a CI rule, make it one and delete the checkbox.

When checklists help

Checklists earn their keep wherever reviewer behavior is inconsistent. New team members learn what the team considers reviewable by seeing the same seven questions asked repeatedly. Microsoft’s study of modern code review ( Bacchelli and Bird) found that understanding the change is reviewers’ top challenge, and a short question list is a comprehension scaffold: it tells you what to try to understand first. Checklists also depersonalize feedback. “Question five: the test asserts nothing about the error case” reads as process, not accusation, and pairing it with the Conventional Comments format (issue, suggestion, nitpick, praise) keeps threads legible. The label does double duty: it tells the author which comments block the merge and which are take-it-or-leave-it, which is half of what review arguments are actually about.

When checklists hurt

The failure mode is the checklist as liturgy: a template pasted into every PR, every box dutifully ticked, no box ever meaning anything. That is not review with extra steps, it is review theatre with better paperwork, and it is arguably worse than honest rubber-stamping because the artifact claims diligence that never happened. The tell is uniformity: when a trivial README fix and a payment-path change produce the same ticked boxes in the same thirty seconds, the checklist has stopped shaping attention and started laundering its absence. Checklists are prompts for thought. The moment compliance becomes the deliverable, delete the template and keep the questions.

Adapt it per tier

Not every change deserves all seven questions at full depth, and pretending otherwise is how the list decays into ritual. Scale it with blast radius: a docs change gets questions one and three; a schema migration or auth change gets all seven, slowly, plus a second reviewer. The checklist is the constant; the depth per item is the dial. Teams that make that dial explicit spend their review hours where wrongness is expensive, and their checklist survives because nobody is forced to perform it where it is pointless. Seven questions, three passes, one dial. That is the whole system, and it fits in your head, which is the only place a checklist ever actually runs.

Frequently asked questions

What should a code review checklist include?

Less than most templates suggest. Seven questions cover the ground: does the change do what it claims, what happens on the unhappy path, will this be understandable in six months, does it fit the existing design, what do the tests actually assert, what breaks downstream, and would you own this line in production. Anything a linter can check should not be on a human checklist.

In what order should you review a pull request?

Three passes: correctness first (does the logic do what the description claims, including failure paths), design second (does the shape of the change fit the system), style last, and only what automation could not catch. Ordering matters because attention is finite; spending it on naming before you have verified the logic is spending your best minutes on the cheapest findings.

Do code review checklists actually improve reviews?

They help most where reviews are inconsistent: new reviewers, growing teams, unfamiliar code. Microsoft’s research found understanding the change is reviewers’ top challenge, and a short checklist keeps attention on the questions that build understanding. They hurt when they become box-ticking: a long template every PR must ceremonially pass turns review into theatre rather than thought.

Junior Developers and AI Code Review

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

The scenario comes straight from Addy Osmani’s writing on agentic code review: a junior developer prompts an agent, gets a plausible five-hundred-line PR, opens it, and it lands. Somewhere in that sequence a question went unasked and unanswered: who actually reviewed this? For junior developers, AI code review is not an abstract workflow debate. It decides whether the review process still teaches them anything, or whether they become couriers moving code they do not understand from a model to the main branch.

The short answer: When junior developers ship AI-generated PRs, review ownership blurs: the junior did not write the code and may not fully understand it, and the senior cannot ask the author why. The fix is norms, not bans. Juniors self-review and annotate before requesting review, seniors review as teaching (why the model chose this, what breaks downstream), and the team treats the review thread as the classroom it has always quietly been.

The ownership gap nobody assigned

In the classic flow, authorship and understanding traveled together. Whoever opened the PR could defend it, and review was a conversation between two people who both held a model of the change. The agent workflow splits that: the junior owns the prompt, the model owns the reasoning, and the reviewer assumes the junior vetted what they are submitting. Often nobody did. Osmani’s framing of the moment is worth repeating: reading code has always been the skill, and it is about to be the skill. The uncomfortable corollary is that juniors are now shipping volumes of code that outrun their reading ability, and the process meant to catch that is the same one being squeezed by the volume.

Review was the learning loop all along

Code review was never only a defect filter. Microsoft’s research on modern code review (Bacchelli and Bird) found that understanding the change is the single biggest challenge reviewers face, and that knowledge transfer is one of review’s most valuable real outcomes, whatever teams claim it is for. We covered the same evidence in do code reviews find bugs: the bug counts are modest, the education is the payoff. For juniors specifically, review comments were the apprenticeship: the place someone senior examined their reasoning and handed back a better mental model. If the code arrives with no reasoning to examine, that loop breaks silently while the merge rate looks healthier than ever.

Seniors: review the model’s choices out loud

The senior reviewer’s job on an AI-heavy junior PR shifts from finding what is wrong to making the invisible reasoning visible. The highest-value comments are questions: why did the model reach for this pattern, what would the alternative have looked like, what does this change do to the callers three files away? Sometimes the junior knows. Often the honest answer is “I did not ask,” and that answer, said in a review thread rather than discovered in an incident, is the teaching moment. A senior who silently fixes or silently approves is optimizing for throughput and training a developer who will need the same rescue next month.

The collaboration cost is already visible

This is not hypothetical hand-wringing. A widely discussed r/ExperiencedDevs thread asks whether AI has made developers less collaborative, and the recurring pattern in the replies is exactly this shape: people consult the model before they consult a colleague, review threads go quiet, and juniors in particular stop asking the questions they used to ask out loud. Every question routed to a model instead of a teammate is individually reasonable and collectively erodes the channel that apprenticeship ran on. Teams do not lose mentorship in one decision. They lose it one skipped conversation at a time.

Norms for junior developers in AI code review

None of this argues for banning agents, which would only push usage underground. It argues for norms that reattach understanding to authorship:

Self-review before requesting review. The junior reads their own diff first, hunk by hunk, and fixes what embarrasses them. We laid out the practice in author self-review; for AI-generated changes it is non-negotiable.
Annotate intent while it exists. What was asked for, what the model chose, what was rejected, what is untested, in PR comments or the description, following capturing intent for AI changes.
The explain-any-hunk rule. If a reviewer points at a hunk, the author can explain it or the PR is not ready. This single norm converts generation speed back into learning pressure.
Keep one review human and conversational. Bots can pre-filter, but a junior’s PR should always get at least one human reviewer who asks at least one why.

The teams that hold these lines get the best of both: juniors who ship faster than any previous generation, and who are actually becoming the seniors who can review what the machines write next.

One more reframe worth stating plainly, again borrowing Osmani’s point: PR size is a review problem before it is a model problem. A junior who prompts an agent into a thousand-line change has not misused the model; they have produced something nobody can teach through. Part of the mentorship now is scoping: helping juniors learn to ask for changes small enough that the review conversation can still happen at all. The skill being trained is not prompting. It is judgment about what a reviewable unit of work looks like, and that skill outlives any particular tool.

Frequently asked questions

Should junior developers use AI to write code?

Yes, banning it just teaches them to hide it. The workable norm is ownership: a junior can generate as much code as they like, but before requesting review they self-review the diff, annotate why the change looks the way it does, and must be able to explain any hunk a reviewer asks about. The tool is fine; unexplained code is not.

How should seniors review AI-generated code from juniors?

Treat the review as teaching, not just defect hunting. Ask why the model chose this approach, what the alternatives were, and what the change breaks downstream. Microsoft’s research on modern code review found understanding is the top challenge and knowledge transfer a core outcome, and those questions are how the transfer survives when a model wrote the first draft.

Does AI-generated code hurt junior developer growth?

It can, if the review loop collapses. Juniors historically learned by having their reasoning examined in review. When an agent writes the code and review becomes a rubber stamp, that loop disappears and juniors can ship for months without forming a model of the system. Teams that keep review conversational, with juniors explaining and annotating their changes, keep the growth.

Multiple AI Code Reviewers: Do Parallel Bots Help?

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

Buried in Addy Osmani’s “Agentic Code Review” is the most interesting experimental result in this space: four AI reviewers run in parallel over the same code flagged issues that were 93.4% unique, with no overlap on the same lines. If multiple AI code reviewers barely duplicate each other, the obvious move is to stack them and harvest the union. The obvious move is also how you drown your team in comments. This piece is about when the ensemble is worth it.

The short answer: Multiple AI code reviewers barely duplicate each other: in the experiment Osmani cites, four parallel reviewers flagged 93.4% of issues uniquely, with no overlap on the same lines. That makes stacking bots tempting for coverage and dangerous for noise, because false positives stack too. Run parallel reviewers only on your highest-stakes paths, keep one tuned reviewer for everything else, and keep a human as the judge of what matters.

The experiment: four reviewers, almost no overlap

The intuition most engineers carry is that review bots are roughly interchangeable: same training data, same failure modes, so a second one mostly restates the first. The result Osmani reports says otherwise. Four reviewers, same diff, and more than nine in ten findings came from exactly one of them. They were not confirming each other; they were looking at different things entirely. Whatever you think of any single bot’s hit rate, that is not what redundancy looks like. It is what independent perspectives look like.

Why multiple AI code reviewers barely overlap

The non-overlap stops being surprising once you drop the mental model of bots as deterministic linters. A linter checks an explicit rule list, so two linters with the same rules converge. A model reviewer is a probabilistic reader whose attention is shaped by its training mix, its context handling, and the prompt wrapped around it. Change any of those and you change what it notices. One reviewer keys on resource handling, another on API misuse, another on the test file nobody else read. Their blind spots differ for the same reason their findings do, which cuts both ways: no overlap also means no cross-confirmation. When two humans flag the same line you gain confidence. When four bots each flag different lines, every finding still arrives with a single vote.

The noise tax is real and it multiplies

Here is the other half of the ledger. The Cotera evaluation ran an AI reviewer over 30 real PRs and got 47 suggestions: 31 were things a linter would catch, 9 were genuinely useful, and 7 were flat wrong. Now multiply that distribution by four parallel reviewers with unique findings. The nine useful comments become a few dozen spread across a much larger pile, and the wrong ones no longer cluster where you can dismiss them together. We wrote about what that does to reviewers in AI review alert fatigue: past a threshold, people stop reading bot comments entirely, and then the ensemble has negative value. Coverage you have trained yourself to scroll past is not coverage.

Where parallel bots actually make sense

The economics work when a missed defect is expensive enough to pay for the triage. That is a tier decision, the same framing we use in review by blast radius:

High-stakes paths. Auth, payments, data migrations, public API contracts. Unique-coverage math favors the ensemble precisely where one missed finding costs more than an hour of comment triage.
Pre-release sweeps. A one-off parallel pass before a major release buys breadth without making noise a permanent feature of every PR.
Everything else: one tuned bot. For routine changes, a single reviewer configured for your codebase, with your suppression list and your severity thresholds, beats three generic ones. Tuning compounds; adding bots does not.

If you do run an ensemble, dedupe and rank findings before any human sees them, and measure each bot’s acceptance rate. A reviewer whose comments are ignored month after month is not a second opinion, it is spam with an API key.

The measurement matters more than the setup. Teams that add a second or third bot rarely remove one, because removal feels like giving up coverage. Track two numbers per reviewer: how often its comments lead to a change, and how often a human explicitly dismisses them. A bot that has not prompted a code change in a month is not protecting you; it is teaching your team that bot comments are ignorable, and that lesson transfers to the bots that are worth reading.

Humans stay the judge

The parallel-reviewer result is genuinely good news, but read it carefully: it is evidence that machine coverage is broader than we assumed, not that judgment parallelizes. Every finding, from one bot or five, is a claim that still needs a human to decide whether it matters here, in this codebase, against this deadline. That division of labor is the same one we argued for in what AI should do in code review: machines to widen what gets noticed, a person to own what gets merged. Stack reviewers where the stakes justify it. Never stack them so high that the person at the end stops looking, because when that happens you have not multiplied review. You have replaced it with the appearance of review, at four times the cost.

Frequently asked questions

Do multiple AI code reviewers find different bugs?

Largely yes. Osmani cites an experiment where four AI reviewers ran in parallel on the same code and 93.4% of flagged issues were unique to a single reviewer, with no overlap on the same lines. Different models and prompts have different blind spots, so heterogeneous reviewers behave less like redundant copies and more like reviewers with genuinely different perspectives.

Should my team run more than one AI review bot?

Only on changes where missed defects are expensive: auth, payments, data migrations, public API surfaces. Parallel reviewers widen coverage but multiply comment volume, and false positives stack just like true findings. For routine changes, one reviewer tuned to your codebase and thresholds produces a better signal-to-noise ratio than three generic ones talking past each other.

How do you stop parallel AI reviewers causing alert fatigue?

Gate them by tier so most PRs see one reviewer or none, dedupe and rank findings before a human reads them, and track the acceptance rate of each bot so chronically ignored ones get removed or retuned. The moment reviewers start skipping past bot comments wholesale, the ensemble is costing attention rather than adding coverage.

Code Review Capacity: Plan It Like a Real Resource

Othman Shareef — Thu, 30 Jul 2026 08:00:00 +0000

Your team plans sprint capacity, on-call rotations, and cloud spend down to the dollar. Almost nobody plans code review capacity, even though it is now the constraint every merge queues behind. Review time is a real, finite, measurable resource: some number of reviewer-hours per week, multiplied by an honest rate at which a human can actually evaluate code. Treat it like the budget it is and a surprising amount of chronic review pain turns out to be simple arithmetic, not a culture problem.

The short answer: Code review capacity is reviewer-hours per week multiplied by an honest reading rate, roughly 400 to 500 lines per hour at the very best. Run that math and most teams discover they can properly review a fraction of what they merge. The fix is not reviewing harder: it is measuring the budget, spending it by risk tier, and watching queue age and rubber-stamp rate as the signals it has run out.

Estimating your code review capacity

Start with reviewer-hours. Not headcount hours: focused, interruptible-only-by-pager hours a person can actually give to reading other people’s diffs. For most engineers that is two to four hours a week before their own work suffers. Then apply a rate. The SmartBear peer review study is the standard reference here: effective review runs at no more than about 500 lines of code per hour, sessions should stay under roughly 60 minutes, and a single sitting should cover at most around 400 lines before defect discovery collapses.

So a five-person team giving three focused hours each has 15 reviewer-hours per week. At the generous 500-lines-per-hour ceiling, that is 7,500 lines of changed code the team can genuinely review. Compare that to what you merged last week. For most teams the merged number is a multiple of the reviewable number, and it always has been. The gap was papered over by skimming.

The ceiling is lower than the math suggests

Those rates are ceilings for finding defects, not targets to hit. Dense business logic reviews slower than generated boilerplate; unfamiliar code reviews slower than code you own. And latency eats capacity from the other side: Google’s review speed guidance asks reviewers to respond within one business day at the outside, because slow turnaround compounds. Authors waiting on review start batching work into bigger PRs, bigger PRs take longer to review, and the queue feeds itself. Capacity planning has to budget for responsiveness, not just total lines.

Spend it by tier, not by arrival order

The default allocation strategy is first come, first served: whatever hits the queue gets whatever attention is left. Deliberate allocation matches attention to risk. A dependency bump and a change to the payment path should not draw from the budget at the same rate. We laid out a concrete tiering scheme in review by blast radius: low-risk changes get automated gates and sampling, high-risk paths get full human attention, and the tier is chosen up front instead of by whoever happens to be tired that afternoon. Once you know your weekly budget is 7,500 lines, deciding which lines get the real hours stops being a philosophical question.

Make the budget visible, too. Teams estimate authoring work in every sprint planning session and never mention the review load that work will generate. A PR heavy sprint should book reviewer-hours the same way it books build time: named people, real hours, counted against their other commitments. When review is an unbudgeted side effect, it is the first thing squeezed and the last thing anyone admits to squeezing.

The AI volume math does not close

This arithmetic was survivable when authorship was the bottleneck. It is not anymore. Osmani, in his essay on agentic review, cites a Faros analysis of 22,000 developers reporting code churn up 861%, defect rates jumping from 9% to 54%, and zero-review merges rising 31.3%. Generation multiplied; review capacity stayed fixed, because it is made of human hours. We walked through the collision in AI writes code faster than you can review: when volume goes up 4x and the budget does not move, the difference is absorbed by shallower review, and nobody announces that decision. It just happens.

Signals the budget is spent

You do not need a dashboard to detect exhausted review capacity, though the metrics are easy to pull. Watch for these:

Queue age. Median time from review request to first response creeping past Google’s one-business-day line, then past two.
Rubber-stamp rate. Approvals landing minutes after the request on diffs that would take an hour to actually read.
Comment density. Substantive comments per hundred changed lines trending toward zero while merge volume holds steady.
Zero-review merges. The share of changes merging with no approval at all, the same number Osmani reports rising 31.3% in the Faros data.

Any one of these means the process still exists on paper while the attention behind it has quietly gone. The honest responses are the ones that change the arithmetic: shrink what needs reviewing, tier what gets the hours, or raise the effective reading rate. That last lever is where tooling belongs (disclosure: we build Pyor for exactly that), because a reviewer who can navigate a diff by risk instead of file order gets more evaluated lines out of the same fixed hour. What no tool can do is make the budget infinite. Plan it like the resource it is, and spend it where being wrong is expensive.

Frequently asked questions

How do you calculate code review capacity?

Multiply the focused review hours your team can realistically give per week by an honest reading rate. Research SmartBear published on peer review puts effective rates under 500 lines of code per hour, with sessions capped near 60 minutes and 400 lines. Five engineers giving three focused hours each is 15 reviewer-hours, or roughly 7,500 well-reviewed lines per week at the absolute ceiling.

What is a realistic code review speed?

Slower than most teams assume. The SmartBear peer review study found defect discovery falls off sharply above 400 to 500 lines per hour, and that a single session should stay under an hour and around 400 lines. Faster reading is possible, but it stops being review and starts being skimming: eyes pass over lines without evaluating them.

What are the signs a team is over its review capacity?

Four show up reliably: review queue age climbing (PRs waiting days instead of hours), rubber-stamp approvals (multi-hundred-line diffs approved in under a minute), comment density trending toward zero, and a rising share of changes merging with no review at all. Each one means the nominal process still exists but the attention behind it is gone.