Hermes Wiki
Developer/DeveloperTools/CodeReview-Linting/Fundamentals/code-review-and-static-analysis-in-ci

Code Review and Static Analysis in CI

Concept

Code quality gets enforced by two complementary mechanisms that operate at different granularities: static analysis (linters, formatters, type-checkers) catches mechanical, rule-based issues automatically and cheaply — a missing semicolon, an unused import, a type mismatch — and human code review catches everything a rule can't express: is this the right design, does this handle the actual failure modes, will the next engineer understand why this exists. Treating them as substitutes for each other wastes both: a human reviewer spending attention on formatting nits that a linter would flag in milliseconds is the most common failure mode, and it crowds out the design-level attention review is actually for.

Google's internal eng-practices guide (later open-sourced and widely adopted outside Google) frames the goal of human review explicitly: the primary purpose is maintaining the long-term health of the codebase, not extracting a perfect diff. Its central standard — "there is no such thing as a perfect CL (changelist); there is only a better CL" — means a reviewer's job is to weigh the value of a requested change against the cost of delaying the author, not to hold every PR to an unreachable bar. Review focus areas are explicitly ranked: design and functionality first (does this belong, does it work), then complexity, tests, naming, comments, and style last — style is exactly the layer a linter should already have caught before a human ever opens the diff.

Tradeoffs

Mechanism What it catches What it can't catch Cost
Static analysis / linting (ruff, ESLint, ty/mypy, tsc) in CI Style violations, dead code, unused variables, many type errors, some real bugs (unreachable code, obvious null-deref patterns) Whether the design is right, whether tests cover the actual failure modes, whether the change should exist at all Near-zero marginal cost per run once configured; the real cost is retrofitting — running a linter for the first time on a large unlinted codebase produces a wall of pre-existing violations that has to be triaged or grandfathered in
Human code review Design fit, correctness against intent, missing edge cases, knowledge transfer, whether the right problem is being solved Anything mechanical a rule could express — humans are worse than linters at consistently catching a missing trailing comma Reviewer time (the scarce resource); larger diffs cost disproportionately more attention per line, not linearly more
No enforced gate (review/lint optional, or local-only) Nothing reliably — depends entirely on individual discipline Everything, inconsistently Cheapest short-term, most expensive long-term: defects that either mechanism would have caught ship instead

The two mechanisms aren't fungible in the other direction either: adding more linting rules doesn't reduce the need for design-level human review, and adding more human reviewers doesn't reduce the value of catching style issues before the diff is even opened. The right split is linting for anything expressible as a rule, review for everything that requires judgment about intent.

When to use / when not to

  • Enforce linting in CI, not just as an editor plugin or pre-commit hook a developer can skip — a check that's only advisory locally gets bypassed under deadline pressure exactly when it matters most.
  • Adopt linting from a project's start where possible; retrofitting onto a large, previously-unlinted codebase should baseline existing violations (fail only on new violations) rather than blocking on a backlog no one has time to fix immediately.
  • Scope human review to diffs a reviewer can actually hold in their head — the empirical anchor here is a widely-cited Cisco/SmartBear study of ~2,500 reviews across 3.2M lines of code: reviews of 200-400 LOC produced the highest defect-detection rates, and defect-finding effectiveness dropped sharply above roughly 400 lines reviewed per hour. A 2,000-line PR isn't reviewed four times as thoroughly as a 500-line one — it's reviewed much less thoroughly per line, because attention doesn't scale linearly with size.
  • Don't require human review to catch what a linter already enforces — reviewers who spend comments on formatting instead of design are a sign the static-analysis gate is under-configured, not that the reviewer is being too picky.

Common pitfall

Treating review speed and review thoroughness as independent, when they trade off directly. The Cisco/SmartBear data above found the inverse relationship explicitly: reviewers who moved faster than ~450 lines/hour had below-average defect-detection in the large majority of cases, while reviewers who stayed under ~400 lines/hour and spent 60-90 minutes on a 200-400 line diff found 70-90% of the defects actually present. Pressure to "unblock" a PR quickly pushes reviewers toward the fast, low-detection end of that curve — the fix isn't asking reviewers to try harder under time pressure, it's keeping diffs small enough that a careful review is also a fast one, and letting static analysis absorb everything that doesn't need a human's judgment in the first place.

Engineering Lens

The real design question for a review process isn't "do we require review" — everyone does — it's where the line sits between what a machine enforces automatically and what a human is asked to judge. A team that lints aggressively and keeps PRs small is optimizing the same total review-time budget differently than a team that lets big PRs through and relies on reviewer diligence to catch everything; the first team gets more consistent defect-detection for the same headcount, because it's not asking human attention to do a rule engine's job. When a review process is producing either rubber-stamp approvals (no real defect-detection happening) or chronic bottlenecks (PRs stuck for days), the diagnosis is almost always diff size and gate placement, not reviewer effort.

Sources

Hermes Wiki