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.
func markSettled(ctx context.Context, transferID TransferID, network string) error {
return db.Transfers.Update(ctx, transferID, TransferUpdate{Status: "settled", Network: 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.
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 returns an error?
func getExchangeRate(ctx context.Context, currency string) float64 {
rate, _ := fxProvider.Lookup(ctx, currency) // error silently discarded
return rate.Value
}
If fxProvider.Lookup times out, or currency isn’t a code the provider supports, rate is the zero value and this function returns 0 — no panic, 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 _.
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.
func getTransfer(w http.ResponseWriter, r *http.Request) {
// router wraps this handler in requireAuth middleware
transfer, err := db.Transfers.FindByID(r.Context(), transferIDFromRequest(r))
if err != nil {
writeError(w, 404, err)
return
}
writeJSON(w, 200, transfer)
}
requireAuth 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.
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
func getActiveAccounts(conn *pgx.Conn) ([]Account, error) {
rows, err := conn.Query(context.Background(), "SELECT * FROM accounts WHERE active = true")
if err != nil {
return nil, err
}
return scanAccounts(rows)
}
// Sound — the caller asks for what it wants, not how to get it
func getActiveAccounts(ctx context.Context) ([]Account, error) {
return accountRepository.FindActive(ctx)
}
The first version forces every call site to acquire and pass around a *pgx.Conn, which means every test needs a real or mocked database connection, and switching databases later means touching every caller. The second version’s callers don’t know or care whether it’s Postgres, an in-memory store, or an HTTP call to another service — that decision stays inside accountRepository, which is exactly where a future change to it belongs.
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.”
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.
// 100% coverage on ApplyInterestRate. Tells you almost nothing.
func TestApplyInterestRate(t *testing.T) {
original := ApplyInterestRate
defer func() { ApplyInterestRate = original }()
ApplyInterestRate = func(balanceCents int64, rate float64) int64 { return 9000 } // stubbed out entirely
if got := ApplyInterestRate(10000000, 0.09); got != 9000 {
t.Errorf("ApplyInterestRate() = %d, want 9000", got)
}
}
That test replaces the exact function it’s supposed to be testing with a stub, so it can never fail — it’s asserting that a stub returns what you told it to return. It’ll show up green in coverage reports forever, including the day someone breaks the real interest calculation, because the real calculation was never actually exercised. 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.
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 nil check?
total := transfer.AmountCents + derefOrZero(transfer.WireFeeCents)
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 nil-pointer panic in production, that some rows really do have WireFeeCents: nil. 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.