Code review has its highest leverage at the design level, not the style level. The questions that matter most are structural.
Are invariants maintained?
What does the system depend on being true, and does this change preserve that? This question catches whole classes of bugs a line-by-line read misses entirely.
// The invariant: a settled transfer always has a settledAt timestamp.
// This PR adds a new status without touching that guarantee.
async function markSettled(transferId: TransferId, network: string): Promise<void> {
await db.transfers.update(transferId, { status: "settled", network });
// settledAt never gets set — nothing enforces it, nothing warns you
}
Line by line, this reads fine: it updates a status, it takes a network, it compiles. The bug only shows up when you ask the invariant question directly — “settled transfers always have a settledAt” was true before this PR and isn’t after it. A line-by-line read has no way to know that invariant exists unless someone states it and checks the diff against it.
Naming the gap in review isn’t the fix by itself — the comment that should come back is “set settledAt here too, and let’s stop this from regressing again”:
async function markSettled(transferId: TransferId, network: string): Promise<void> {
await db.transfers.update(transferId, {
status: "settled",
network,
settledAt: clock.now(),
});
}
And the regression test that pins the invariant so the next PR that drops settledAt fails CI instead of shipping:
test("markSettled sets settledAt", async () => {
// ...set up a pending transfer...
await markSettled(transferId, "swift");
const got = await loadTransfer(transferId);
if (got.status === "settled" && got.settledAt === null) {
throw new Error("settled transfer has no settledAt");
}
});
Asking the invariant question in review catches the bug; a test like this is what stops it from coming back in six months under a different PR.
Are trade-offs stated explicitly?
Every engineering decision sits on a spectrum: efficiency versus thoroughness, optimality versus brittleness, speed versus correctness. A mature PR states the constraint upfront: “this is faster, but it requires X to hold.” Watch for the retrospective version instead, defending “if only we’d had more time” after the fact. That’s rationalization, not a trade-off.
PR description, version A: “Caches the interest-rate lookup for 5 minutes. This assumes rates don’t change more than once every 5 minutes — if treasury starts doing intraday rate changes, we’ll need a shorter TTL or cache invalidation on write.”
PR description, version B: “Added caching for performance.”
Both PRs might contain the identical diff. Version A gives you something to review: is 5 minutes actually safe, does anyone know about upcoming intraday-rate plans, is there a monitoring signal for stale rates? Version B gives you nothing to push back on until the stale-rate bug ships and someone asks “wait, why did we cache this?” — at which point the honest answer is usually “we didn’t think about it,” dressed up after the fact as “we didn’t have time.”
Are failure modes handled?
What happens when the network is slow, the database returns an error, the input isn’t what you expected? A happy-path-only implementation isn’t done.
// Happy-path only — what happens when lookup rejects?
async function getExchangeRate(currency: string): Promise<number> {
const rate = await fxProvider.lookup(currency).catch(() => undefined);
return rate?.value ?? 0; // silently falls back to 0 on any failure
}
If fxProvider.lookup times out, or currency isn’t a code the provider supports, the catch swallows it and this function returns 0 — no exception, no error, just a silently wrong exchange rate flowing into whatever calls it next. The question isn’t “does this work” — the demo will always work. It’s “what happens to a currency conversion when the FX provider is down,” and if nobody can answer that in the PR, the failure mode wasn’t handled, it was just discarded with catch(() => undefined).
Are the auth and authz boundaries correct?
Can a user reach data they shouldn’t after this change? Is every access gated on the right permission check? Auth bugs slip in easily and hide well.
// Authenticated — but not authorized. Any logged-in user can fetch any transfer.
// router applies requireAuth middleware before this handler runs
async function getTransfer(req: Request, res: Response) {
const transfer = await db.transfers.findById(transferIdFromRequest(req));
if (!transfer) {
return res.status(404).json({ error: "not found" });
}
res.status(200).json(transfer);
}
requireAuth is middleware the router applies before this handler runs — it reads the session token, confirms it’s valid, and rejects the request with a 401 if not. That’s all it checks: that someone is logged in. It says nothing about whether this someone owns this transfer. The endpoint works perfectly in every manual test a developer runs, because they’re always testing against their own transfers. It fails exactly the test nobody runs by accident: log in as customer A, request customer B’s transfer ID. That gap between “authenticated” and “authorized” is the single most common shape auth bugs take, and it’s invisible unless someone reviewing the PR asks the ownership question directly.
The fix adds the ownership check the middleware never did:
async function getTransfer(req: Request, res: Response) {
const transfer = await db.transfers.findById(transferIdFromRequest(req));
if (!transfer) {
return res.status(404).json({ error: "not found" });
}
if (transfer.customerId !== currentCustomerId(req)) {
// 404, not 403 — don't confirm the ID exists to a non-owner
return res.status(404).json({ error: "not found" });
}
res.status(200).json(transfer);
}
Returning 404 instead of 403 for a transfer that exists but isn’t the caller’s is deliberate: a 403 confirms the ID is valid and belongs to someone, which is itself information an attacker probing IDs shouldn’t get for free.
Is the interface sound?
Will callers need to know more about internals than they should? The right abstraction multiplies productivity; the wrong one creates friction. An interface that leaks implementation details makes every future change more expensive than it needs to be.
// Leaky — every caller now needs to know this is backed by Postgres
async function getActiveAccounts(conn: pg.Client): Promise<Account[]> {
const { rows } = await conn.query("SELECT * FROM accounts WHERE active = true");
return scanAccounts(rows);
}
// Still leaky — accountRepository is a module-level singleton, so every test
// that imports this module links against whatever it's set to, and there's
// still exactly one implementation any caller can get.
async function getActiveAccounts(): Promise<Account[]> {
return accountRepository.findActive();
}
// Sound — the caller asks for what it wants, not how to get it, and the
// service owns its dependency instead of reaching for a singleton.
class AccountService {
constructor(private readonly accounts: AccountRepository) {} // an interface, injected at construction
async getActiveAccounts(): Promise<Account[]> {
return this.accounts.findActive();
}
}
The first version forces every call site to acquire and pass around a pg.Client, which means every test needs a real or mocked database connection, and switching databases later means touching every caller. The second version fixes the leaky signature but not the leaky dependency: accountRepository is still a fixed, module-level thing, so a test can’t swap in a fake without mutating shared state, and nothing about the function’s signature says it needs a repository at all. The third version’s callers don’t know or care whether accountRepository is backed by Postgres, an in-memory store, or an HTTP call to another service — that decision is made once, at construction, by whoever wires up AccountService, and a test can hand it an in-memory fake with a one-line constructor call.
What does this mean for the data model?
If this touches a schema, is the migration safe under concurrent writes? Is it backward compatible? Is the new shape actually correct, or just convenient for how the feature works today?
-- Looks reasonable in isolation
ALTER TABLE accounts ADD COLUMN risk_tier TEXT NOT NULL;
On an empty table this is instant. On a production accounts table with ten million rows, NOT NULL with no default means Postgres has to rewrite every existing row before the migration completes — and depending on version and lock mode, it can hold a lock that blocks writes to the table for the entire duration. The fix (add nullable, backfill in batches, then add the constraint) isn’t obscure, but it’s invisible if the reviewer only asks “does this get me the column I need” instead of “what does running this against the real table actually do.” Safe Operations For High Volume PostgreSQL is a good reference for the general pattern: which DDL operations take a table-wide lock, and how to get the same end state without one.
Is the test quality real?
A test that mocks every dependency and only checks the happy path buys you false confidence. A good test fails for the right reason when behavior changes. Coverage numbers say nothing about whether that’s true.
// AccountService.applyInterestRate just delegates to this.interest.calculate(...)
const stubInterestEngine: InterestEngine = {
calculate: (balanceCents: number, rate: number) => 9000,
};
// 100% coverage on applyInterestRate. Tells you almost nothing.
test("applyInterestRate", () => {
const svc = new AccountService(stubInterestEngine);
expect(svc.applyInterestRate(10_000_000, 0.09)).toBe(9000);
});
The stub already knows the answer before the test runs — it returns 9000 no matter what balance or rate comes in, and applyInterestRate just hands those arguments straight through to it. The real interest math, wherever it actually lives, never runs. This test can’t fail unless someone changes the stub or the delegation itself, so it’ll show up green in coverage reports forever, including the day someone breaks the real calculation. A reviewer skimming for “is there a test” would approve this. A reviewer asking “would this test go red if the logic broke” wouldn’t — and would ask where the test that actually exercises interest.calculate’s real implementation lives.
Could you maintain this in six months?
Is the intent clear to someone who wasn’t in the room for the planning discussion? If you had to change this without any context, what would trip you up?
// Six months from now, is this obviously safe to delete the ?? fallback?
const total = transfer.amountCents + (transfer.wireFeeCents ?? 0);
Today, the person who wrote this knows wireFeeCents was added last month and old transfers in the database predate the column, hence the fallback. In six months, with no comment and no context, this reads like defensive programming against something that might not even be possible anymore — and the next person either leaves it forever out of caution, or simplifies it to transfer.amountCents + transfer.wireFeeCents and finds out the hard way, with a NaN leaking into production, that some rows really do have wireFeeCents: null. One sentence in the PR — “old transfers don’t have wireFeeCents set, this is the migration-safe default until the backfill lands” — is the difference between a maintainable line and a landmine.
The checklist
The eight questions, stripped of their examples, for pasting into a PR template or keeping open in a second tab:
- Invariants. What does this system depend on being true? Does this change preserve it, and is there a test that would catch the next regression?
- Trade-offs. Does the PR state its assumptions upfront, or only after something breaks?
- Failure modes. What happens when the network is slow, the dependency errors, or the input is malformed?
- Auth boundaries. Is this access gated on this user, this resource — not just “is someone logged in”?
- Interface soundness. Do callers need to know more about internals than they should? Can a caller unit-test against this without a real dependency?
- Data model. If this touches a schema, is the migration safe under concurrent writes and production-scale data?
- Test quality. Would this test actually fail if the logic broke, or does it just exercise a mock?
- Six-month maintainability. Would the next person, with no memory of this conversation, understand why this line exists?