Code Review and Working with Existing Codebases Questions

Direct answer

At org scale, "review faster" doesn't work, the fix is redesigning how review load is distributed: automate everything mechanical before a human ever opens a diff, route each pull request (PR, a proposed code change submitted for review) to the right owner instead of a free-for-all queue, triage by risk so low-risk changes take a lightweight path, and make reviewer load a metric leadership actually watches, the same way they'd watch on-call load.

Structured elaboration

Automation removes load before a human sees the diff

• Continuous integration (CI, the automated build/test pipeline) blocks on lint, type checks, unit tests, and coverage delta, so reviewers never comment on things a machine already caught
• A bot flags PR size and touched-file risk (for example, "this modifies auth middleware") so a reviewer knows what they're walking into before opening it

Triage by risk

• Classify changes into tiers: low-risk (docs, config, isolated feature-flagged code) gets a single reviewer or even auto-merge after CI passes; medium risk gets normal review; high-risk (shared libraries, security, data migrations, billing) gets a mandatory named owner plus a second reviewer
• This concentrates scarce senior reviewer attention on what actually needs it instead of spreading it thin and evenly

Reviewer assignment and code ownership

• CODEOWNERS-style routing (a config file mapping directories to the team or person who owns that code) auto-assigns based on what the PR actually touches, instead of a random queue
• Load-balance assignment within an ownership group (round robin or least-loaded) instead of everyone requesting the one person known to be thorough
• Rotate a weekly "on-call reviewer" role per team so load doesn't permanently concentrate on the same two or three people

Preventing burnout while cutting time-to-merge

• Cap how many PRs a single reviewer is expected to have open at once, visible on a shared dashboard
• Protect a daily review block instead of letting review compete with deep work as constant interrupts
• Track reviewer load as a first-class metric leadership actually looks at, not an afterthought

Worked example

An org of 300 engineers has PR latency (open to merge) at a median (the middle value, half of PRs merge faster and half slower) of 3 days, driven by 15% of reviewers absorbing 60% of review volume, the people everyone requests because they're known to be thorough. The plan: introduce CODEOWNERS-based auto-assignment for the 20 highest-traffic directories, add a bot that classifies PRs into risk tiers from touched paths and diff size, and set a rule that low-risk PRs need one reviewer with a 4-business-hour service-level agreement (SLA, an explicit response-time target) while high-risk PRs need a named owner with a 1-business-day SLA. Leadership adds a dashboard showing open-review-count per person, and anyone over a threshold (say, 8 open reviews) gets rebalanced by their lead. After a quarter, median latency for low-risk PRs drops sharply because they no longer queue behind high-risk items, and concentration on the top reviewers eases because ownership-based routing spreads assignments across each team instead of funneling everything to a few known-good individuals.

Trade-offs and pitfalls

• Automated risk-tiering can misjudge risk (a one-line change to a rate limit is "small" but dangerous), keep a human override on the classification
• CODEOWNERS routing can create silos where only the "owner" ever reviews a path, losing the cross-pollination review normally provides; rotate ownership periodically
• Cutting time-to-merge by lowering the bar (fewer required reviewers everywhere) trades quality for speed; the real lever is distributing existing rigor better, not removing it
• A reviewer-load dashboard used punitively instead of for rebalancing damages trust fast; be explicit it's a load-balancing signal, not a performance metric

Direct answer

Treat this review as three linked jobs: prove the change doesn't break the existing contract, or clearly flag that it intentionally does; have tests that catch a break automatically rather than relying on a human noticing during review; and if it IS a breaking change, run an explicit communication and coordination process across every downstream team before it ships, not just a changelog entry after the fact.

Structured elaboration

Verify backward compatibility. Diff the public API (application programming interface) surface, endpoints, request and response schemas, public method signatures, against the previous version using an automated schema-diff tool. Check that the semantic-versioning bump (version numbers like 2.1.0, where the first number only increases for a breaking change) actually matches the impact: a removed field or endpoint is a major-version break, not a minor one. Manually scan for the classic breaking patterns an automated diff might present too dryly to flag on its own: a field that used to be optional becoming required, a removed enum value, or a changed error code that a consumer's error-handling logic depends on.

Tests to require. Unit and integration tests for the new behavior, plus consumer-driven contract tests: each known consumer publishes a "contract" describing what it expects from this API, and the provider's continuous integration (CI) pipeline runs every consumer's contract against the new code before merge, so a break shows up as a failing build instead of a production incident weeks later.

Documenting and communicating an intentional break. If a break is genuinely necessary, it needs a migration guide with concrete before-and-after request or response examples, a deprecation timeline (announce now, remove after an agreed grace period), and a version bump that signals the change to anyone watching semantic versioning.

The multi-team angle. This is the part that's easy to skip. Different downstream teams often run on different release cadences: one team deploys daily, while another ships a mobile client that goes through app-store review on a multi-week cycle. "We announced it in the changelog" is not the same as "every consumer had a realistic window to migrate before the old behavior disappeared." The reviewer's job includes checking that the deprecation window is long enough for the SLOWEST consumer's release cadence, not the average one, and that there's a direct, tracked notification, not just a changelog line, to every known downstream team: who owns each consumer, has that owner acknowledged the timeline, and is there a release-coordination step, such as a shared rollout calendar or a required sign-off from each consuming team, before the old behavior is actually removed.

Worked example

A concrete scenario: a payments API removes a deprecated legacy_currency_code field. Applying the process: an automated schema diff confirms the field is gone, which requires a major-version bump. Contract tests run for the three known consumers (a web checkout, a mobile app, and a partner integration), and the partner integration's contract still references the old field, so its test currently fails, that failure is the exact signal that should hold the release, not something to work around. A migration guide is written showing the old request shape next to the new one. The deprecation timeline is set to match the partner integration's slowest release cadence, which, per that team, needs six weeks of notice because their deploys go through a change-advisory process, not the web team's daily-deploy pace. Each of the three consuming teams gets a direct message naming the field, the deadline, and a link to the migration guide, and the field's actual removal is gated on each team explicitly acknowledging the change, not merely being notified of it.

Trade-offs and pitfalls

Coordinating across three teams' release cadences is slower than shipping the break and hoping consumers keep up, but the alternative, a downstream team's production system silently breaking because nobody saw a changelog entry, is far more expensive to unwind afterward. A common pitfall is confusing "we deprecated it" with "it's now safe to remove": the deprecation period only did its job if every consumer actually acted on it, which needs active follow-up, for example a dashboard tracking remaining callers of the old field, rather than passively waiting out the clock.

Direct answer

Don't let the disagreement stay abstract: get concrete about what's actually at stake, how many callers exist and how costly a rename would be later, state your reasoning once clearly, and if it's still unresolved, use a fast, low-cost tiebreaker (a quick prototype, a third opinion, or an explicit decision-maker) rather than letting the thread drag on.

Structured elaboration

1. Separate preference from cost. Ask explicitly whether this is reversible. An API used by many callers is expensive to rename later, a breaking change or a long deprecation, so the bar for insisting on a preferred name should scale with how many callers exist and how costly a change would be, not with how strongly either person feels.
2. State your reasoning once, concretely, and ask for theirs. Name the specific downside, not just "I don't like this name": "getUser reads as a lookup that returns null, but this always creates one if missing, that's surprising for the many call sites that assume a pure read."
3. If still unresolved, do a quick, cheap check instead of more debate. Grep the existing callers to see how the API is actually used, or spend ten minutes prototyping both names in real call sites to see which reads better. This replaces opinion with evidence.
4. If still stuck, get a third opinion fast. A tech lead, or whoever owns that part of the API, makes the call; anyone involved can request this, and it's cheap to ask compared to letting the PR (pull request) stall.
5. Once decided, commit and close the thread visibly. Whoever didn't get their preference says so out loud ("okay, going with X") so the disagreement doesn't linger as an unresolved grudge, and the reasoning gets written down briefly (in the PR or a short doc) so the next naming debate doesn't restart from zero.

Worked example

Two engineers disagree on getUser versus findOrCreateUser for a method with roughly forty existing call sites. The reviewer greps the callers and finds most assume a pure lookup with no side effects. Bringing that evidence back, "most callers don't expect this to write anything, the name should signal that," the author agrees the evidence changes the calculus, and they land on findOrCreateUser. A short note is added to the PR explaining the reasoning so a future reader understands why the name is what it is.

Trade-offs and pitfalls

Escalating to a tech lead for every minor naming choice trains the team to stop deciding things themselves. Conversely, letting every naming disagreement burn hours of back-and-forth stalls delivery on something that's often genuinely reversible later behind an alias. The failure mode to avoid is a reviewer pulling rank, "just do it my way, I'm the reviewer," instead of showing the reasoning; that resolves the immediate PR but erodes trust for the next one.

Direct answer

Below is a script that reads git diff --numstat from STDIN (standard input, the default input stream a program reads from when data is piped into it) and emits one JSON object with total additions, total deletions, files changed, and any files whose additions exceed 200. Wiring it into continuous integration (CI, the automated build/test pipeline that runs on every push) means piping the pull request (PR)'s diff into the script and using its output to apply a label through the CI platform's API.

Structured elaboration

Approach

• git diff --numstat emits one tab-separated line per changed file: <added>\t<deleted>\t<filename>
• Binary files report - for added and deleted instead of a number, this must be handled explicitly rather than crashing the parser
• Parse each line, accumulate totals, and separately collect filenames whose additions exceed the 200-line threshold

Code

import sys
import json


def parse_numstat(lines):
    total_additions = 0
    total_deletions = 0
    files_changed = 0
    large_files = []
    for line in lines:
        line = line.rstrip("\n")
        if not line.strip():
            continue
        parts = line.split("\t")
        if len(parts) != 3:
            continue
        added_str, deleted_str, filename = parts
        files_changed += 1
        # binary files report '-' for added/deleted; treat as 0 rather than crash
        added = int(added_str) if added_str != "-" else 0
        deleted = int(deleted_str) if deleted_str != "-" else 0
        total_additions += added
        total_deletions += deleted
        if added > 200:
            large_files.append(filename)
    return {
        "total_additions": total_additions,
        "total_deletions": total_deletions,
        "files_changed": files_changed,
        "large_files": large_files,
    }


def main():
    lines = sys.stdin.readlines()
    result = parse_numstat(lines)
    print(json.dumps(result))


if __name__ == "__main__":
    main()

CI integration

• In CI, compute the numstat against the PR's actual merge base (for example git diff --numstat origin/main...HEAD, or the CI platform's own base-comparison ref) and pipe it into the script
• Parse the JSON output and use a non-empty large_files list, or total_additions crossing a team-chosen threshold, to apply a "large-pr" label through the CI platform's API (for example the GitHub CLI's gh pr edit --add-label, or a direct REST call)
• Run this as an early, cheap CI step, before slower checks, so the label is visible to reviewers as soon as the PR opens

Worked example

I ran this exact script against the following sample git diff --numstat input:

5	2	src/app.py
0	0	README.md
250	10	src/big_module.py
-	-	assets/logo.png

Piped through the script (... | python3 diffstats.py), the actual output was:

{"total_additions": 255, "total_deletions": 12, "files_changed": 4, "large_files": ["src/big_module.py"]}

That matches the arithmetic by hand: additions 5 + 0 + 250 + 0(binary) = 255, deletions 2 + 0 + 10 + 0(binary) = 12, four lines means four files changed, and only src/big_module.py has additions (250) over the 200 threshold.

Trade-offs and pitfalls

• numstat reports - for binary files; treating that as 0 additions is a deliberate choice so the parser doesn't crash, but it means a genuinely large binary change (a big generated asset) won't trip the large-file flag
• A pure line-count threshold doesn't capture every sense of "large": 250 lines of an auto-generated config file is not the same risk as 250 lines of hand-written logic; use this as a first-pass signal, not the only one
• Commits within the PR don't matter to this script, since it's comparing the merge base to the tip, which is usually what you want for a PR-level label
• If the base ref is stale (a local branch that hasn't been rebased), the numbers will be wrong; make sure CI diffs against the PR's true merge base, not a stale local reference

Direct answer

Constructive review feedback that preserves psychological safety (a shared sense that it's safe to be wrong or imperfect without punishment) comes down to a handful of concrete, repeatable practices: address the code rather than the person, lead with intent, label severity honestly, give real praise, offer a concrete alternative, and leave the door open on next steps.

Structured elaboration

At least six concrete practices, and why each helps the author actually receive and act on the feedback:

1. Phrase it about the code, not the person ("this function doesn't handle X" rather than "you forgot X"). It keeps the comment about the artifact, which is easier to hear without feeling personally judged.
2. Lead with a question or the underlying intent ("what happens if the list is empty here?" instead of "you missed the empty case"). It invites the author to reason it through rather than just comply, and softens the tone.
3. Label severity explicitly (blocking versus a "nit:" versus optional). It removes the guesswork of whether every comment is a must-fix, which reduces the feeling of being buried under criticism.
4. Include genuine, specific praise, not filler. It reinforces what to keep doing and signals the review isn't only a list of what's wrong.
5. Give a concrete example or alternative, not just "this is unclear." A vague criticism with no path forward reads as judgment; a concrete suggestion reads as help.
6. Offer next steps when there's no obvious fix ("happy to pair on this if useful"). It shows the reviewer is invested in the outcome, not just gatekeeping.
7. Time the delivery, avoiding a flood of stylistic comments while the core design is still in question, since dozens of comments landing at once reads as harsher than any single one intended.

Worked example

A function is missing a null check. A comment that violates most of these practices: "this is wrong, add a null check." A comment applying several practices at once: "nice catch handling the retry case above! One thing: what happens if user is null here, e.g. a deleted account mid-request? Might be worth an early return. Happy to pair if useful." Same underlying concern, delivered in a way the author can act on without feeling attacked.

Trade-offs and pitfalls

Over-softening a genuinely blocking issue ("just a thought, feel free to ignore") creates ambiguity about severity, and the issue can ship anyway. Psychological safety is not the same as avoiding disagreement; being clear that something is blocking is itself respectful, because it's honest rather than vague.