backend / code quality / 04_pre_commit_and_review.md

Pre-commit Hooks and Code Review

6 interview angles 6 min read source

Pre-commit Hooks and Code Review

The two gates between “code written” and “code merged.” Pre-commit catches the mechanical stuff automatically; review catches the judgment stuff. A senior engineer sets up both so the team’s bar holds without nagging.

Pre-commit hooks

The pre-commit framework runs checks on git commit (and in CI), so mechanical problems never reach review.

# .pre-commit-config.yaml
repos:
  - repo: https://github.com/astral-sh/ruff-pre-commit
    rev: v0.6.0
    hooks:
      - id: ruff          # lint
      - id: ruff-format   # format
  - repo: https://github.com/pre-commit/mirrors-mypy
    rev: v1.11.0
    hooks:
      - id: mypy
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.6.0
    hooks:
      - id: trailing-whitespace
      - id: end-of-file-fixer
      - id: check-yaml
      - id: check-added-large-files
      - id: detect-private-key      # block committing secrets

What belongs in pre-commit:

  • Formatting (ruff-format / black) — auto-fixable; no human should ever comment “fix the indentation.”
  • Linting (ruff) — unused imports, obvious bugs, style violations.
  • Type checking (mypy/pyright) — though some teams run this CI-only because it can be slow.
  • Secret detection (detect-private-key, gitleaks) — block credentials before they enter git history.
  • Cheap structural checks — large files, merge-conflict markers, broken YAML/JSON.

Principles:

  • Fast. Pre-commit should be seconds. Slow hooks get bypassed with --no-verify. Run on changed files only (the framework does this); push the slow stuff (full test suite) to CI.
  • Run the same checks in CI. Pre-commit is a convenience, not a gate — developers can skip it. CI runs the identical hooks (pre-commit run --all-files) as the actual gate.
  • Auto-fix where possible. A formatter that fixes the file is better than a linter that just complains.
  • Pin hook versions (rev:) so the checks are reproducible across machines and over time.

Code review — what it’s actually for

Pre-commit and CI handle the mechanical: formatting, lint, types, tests pass. So review is freed to focus on the judgment — the things a tool can’t check.

What to look for, roughly in priority order:

  1. Correctness — does it do what it claims? Edge cases, error paths, off-by-ones, race conditions.
  2. Design fit — does it belong here? Is it the right abstraction, or a hack that’ll rot? Does it duplicate something that exists?
  3. Failure modes — what happens when the DB is down, the input is malformed, the dependency times out? Is it idempotent if it needs to be?
  4. Security — injection, missing authz check, secret in code, unsafe deserialization.
  5. Tests — do they test behavior (not implementation)? Do they cover the risky paths? Would they catch a regression?
  6. Readability — will the next person understand this? Naming, structure. (But not style — the formatter owns that.)
  7. Operational — logging, metrics, does it degrade gracefully, is it observable?

What review should not be: a place to argue about formatting (the tool decided), or to rewrite the author’s approach because you’d have done it differently (only if it’s actually wrong, not just different).

Giving review feedback well

  • Distinguish blocking from non-blocking. Prefix or label: “blocking: this can deadlock” vs “nit: minor naming” vs “question: why this approach?” The author shouldn’t have to guess what stops the merge.
  • Comment on the code, not the person. “This function does X and Y — splitting would help” not “you always write functions that do too much.”
  • Explain the why. “Use for_each here — count would recreate everything if a middle element is removed” teaches; “use for_each” just instructs.
  • Ask, don’t assume. “What happens if items is empty here?” surfaces the bug and respects that the author may know something you don’t.
  • Approve with minor comments when the only issues are nits — don’t hold up a merge over naming. Trust the author to address them.
  • Praise good things occasionally — a clean refactor, a clever test. Review that’s only criticism trains people to dread it.

Receiving review feedback well

  • Separate the message from the delivery. Even bluntly-worded feedback often has a valid point.
  • Don’t defend reflexively — confirm you understood before pushing back. Half the time you realize they’re right.
  • Disagree with reasons, in the open — “I considered that; I chose this because X” is fine. Resolve it in the thread, not by silently ignoring.
  • It’s the code being reviewed, not you.

Review logistics that matter

  • Small PRs. A 50-line PR gets a real review; a 2,000-line PR gets a rubber stamp. The single biggest lever on review quality is PR size.
  • PR description does the framing — what changed, why, how to test, what to look at. The reviewer shouldn’t reverse-engineer intent.
  • Fast turnaround. A PR sitting for two days blocks the author and goes stale. Review is a priority, not a “when I get to it.”
  • Author self-reviews first — read your own diff before requesting review; you’ll catch the obvious stuff yourself.

CI as the actual gate

Pre-commit is local convenience; CI is the enforcement:

on: pull_request
jobs:
  checks:
    - run: pre-commit run --all-files   # same hooks, now as a gate
    - run: mypy myapp/
    - run: pytest

Branch protection: require the CI checks to pass and require an approving review before merge. Now the bar holds structurally, not by people remembering.

Common gotchas

  • Slow pre-commit hooks — developers bypass with --no-verify. Keep it seconds; full tests go to CI.
  • Pre-commit without CI enforcement — it’s skippable, so it’s not a gate. CI must run the same checks.
  • Reviewing formatting — wasted human attention on what a tool owns. Auto-format; review judgment.
  • Huge PRs — get rubber-stamped. Small PRs get real review.
  • No blocking/non-blocking distinction — the author can’t tell what actually stops the merge.
  • Feedback on the person — “you always…” trains defensiveness. Comment on the code.
  • Review as a bottleneck — PRs sitting for days. Fast turnaround keeps the team moving.

Interview angle

  • “What belongs in pre-commit vs CI?” — pre-commit: fast, changed-files-only, auto-fixing checks (format, lint, secret detection) — local convenience. CI: the same checks plus the slow ones (full type check, test suite) — and CI is the actual gate, because pre-commit is skippable with --no-verify.
  • “What do you look for in a code review?” — correctness and edge cases first, then design fit, failure modes, security, test quality, readability, operability. Not formatting or style — the tooling owns that, which frees review for the judgment a tool can’t make.
  • “How do you give review feedback?” — label blocking vs nit vs question so the author knows what stops the merge; comment on the code not the person; explain the why so it teaches; ask questions rather than assume; approve-with-nits instead of blocking on trivia.
  • “What’s the biggest lever on review quality?” — PR size. A small PR gets a real review; a 2,000-line PR gets a rubber stamp. Small, focused PRs with a clear description that frames intent.
  • “How do you make the team’s quality bar hold without nagging?” — automate the mechanical (pre-commit + CI running identical checks), gate merges on CI + an approving review via branch protection, keep PRs small, and reserve human review for judgment. The bar holds structurally instead of depending on people remembering.
  • “How do you receive tough feedback?” — separate the message from the delivery (bluntly-worded feedback often has a real point), confirm you understood before pushing back, disagree with reasons in the open rather than silently ignoring, and remember it’s the code under review.
Previous
Type-Checking Strategy