Change review

Applies when: The change was written fully or partly by an AI agent.

Rule. Verify that every import and external call the agent wrote exists with that signature.

Why. Agents invent plausible APIs and rewrite nearby utilities without searching. A study of 16 code models found that at least about 5% of the packages they suggested (about 22% for open-source models) did not exist, and an attacker can publish a malicious package under a name that models keep inventing. A green build proves only what the compiler sees. chg-reuse-before-write covers re-implemented helpers. The author must also be able to explain each hunk; see chg-agent-independent-review.

import requests_cache_pro                # package does not exist, or exists only because an attacker registered it
total = sum_cents(items)                 # new helper; money.sum_cents() already exists
resp = client.fetch_json(url, retries=3) # fetch_json has no retries param

from money import sum_cents
resp = client.fetch_json(url)   # signature checked against source

Counterpoint. "Humans make the same mistakes." True, but agents make them fluently and at volume, and disclosure alone does not catch them. Kubernetes requires disclosure and an author who can explain the change. A compiler and a dependency check catch the mechanical part, so use this card for calls that resolve but behave differently than assumed.

Tool gate: Compiler, type checker and dependency-resolution in CI catch non-existent imports and calls.

Sources: www.kubernetes.dev, google.github.io, arxiv.org

Applies when: An agent wrote the change, and an agent or the same session is asked to review it.

Rule. Get sign-off from a person or a fresh-context reviewer who has the spec but not the author's reasoning; self-review by the writing session does not count.

Why. The same context repeats the same blind spots and assumptions. An approval without real reading is a rubber stamp. The person who submits the change must also be able to explain each hunk.

Session writes the diff, then "reviews" it: "no issues found". Merged.

Fresh reviewer given spec + diff + test output, runs the branch, files findings.
A human who can explain the change approves.

Counterpoint. A same-session pass still catches typos and leftovers. Keep it as the author's self-review step, not as the gate. Meta tracks "eyeball time" as a guardrail against rubber-stamping, the failure this card targets.

Sources: www.kubernetes.dev, engineering.fb.com, stripe.dev, www.uber.com, dropbox.tech

Applies when: Tests were generated or edited alongside the code under test, especially by an agent.

Rule. Write tests that fail when the behaviour breaks, and reject tests that mostly assert on mocks, copy the implementation, or were edited to match new output without explanation.

Why. Generated tests tend to confirm what the code does, so they pass against a bug. A suite that cannot fail gives false confidence. Mock only at real boundaries; see mod-mock-at-boundaries-only.

repo = Mock(); repo.save.return_value = 1
assert svc.create(u) == 1          # asserts the mock's own answer
repo.save.assert_called_once()

svc = Service(InMemoryRepo())
svc.create(u)
assert svc.get(u.id).email == u.email   # fails if create loses data

Counterpoint. Mocks isolate slow dependencies. Mock at real boundaries and still assert on observable outcomes. To check a test, break the code deliberately once: if the test stays green, it fails this card. Test messages should show the input, the actual value, and the expected value.

Tool gate: Mutation testing; fail the build if mutated code still passes.

Sources: google.github.io, go.dev, michaelfeathers.silvrback.com, github.com, github.com

Applies when: The diff adds, removes or changes an HTTP route, a request or response field, a status code or a header.

Rule. Update the API spec in the same change that alters the routes, fields, status codes or headers it describes.

Why. A spec updated "later" is usually never updated. The reviewer also loses the one place where the shape of the change is readable at a glance.

diff: + @app.post("/orders/{id}/cancel")
      (no change to openapi.yaml)

diff: + @app.post("/orders/{id}/cancel")
      + openapi.yaml: /orders/{id}/cancel with 204, 404 and 409 responses

Counterpoint. "The spec is generated, so every change updates it." Then the generated file is committed or checked in CI, and the diff shows it. If it is generated at build time and not committed, mark the card N/A. svc-api-spec-matches-impl checks the service as a whole.

Tool gate: Breaking-change diff of the spec in CI, plus a check that fails when route files change and the spec file does not.

Sources: spec.openapis.org, opensource.zalando.com

Applies when: A review approval or verification result exists and the branch has been pushed or rebased since.

Rule. Treat an approval or verification verdict as current only if no push since then changed non-test content, and compare patch ids after a rebase.

Why. A verdict covers one patch. After a push or rebase the reviewer approved code that no longer exists.

Approved at abc123. Two later pushes change src/auth.py. Merged on the old approval.

Approved at abc123. Rebase onto main: `git patch-id` matches for every changed file, so the approval stands.
Push that edits src/auth.py: approval dismissed, re-requested.

Counterpoint. "Re-approving after every push is noisy." Only a change to non-test content voids the verdict, and a rebase that keeps the patch id does not. Branch protection can enforce this without human effort.

Tool gate: Branch protection that dismisses stale approvals on new commits.

Sources: github.com

Applies when: The change removes, rewrites or "simplifies" odd-looking existing code, or rewrites a module.

Rule. Add a characterization test before rewriting code you do not understand.

Why. Strange code often carries a fix for a bug that is no longer visible. Removing it re-ships the bug. Check the history first (blame, the PR that added it, tickets, postmortems); defensive code such as null checks, retries and timeouts usually answers a real incident. For a rewrite, name the defect that justifies it.

PR "simplify retry loop": removes sleep(0.2) and a dedup check.
No blame/history cited; no test. Rewrite justified by "this is a mess".

git blame shows sleep added for rate-limit incident INC-17.
PR keeps it with a comment, adds a characterization test first.

Counterpoint. Joel Spolsky's anti-rewrite case rests on anecdotes (Netscape, Borland), and Hickey argues for restating the problem, which may mean a new design. Rethinking is allowed. A big-bang rewrite needs small shippable steps, the old path working meanwhile, and a named defect.

Sources: www.joelonsoftware.com, michaelfeathers.silvrback.com, github.com, github.com

Applies when: The change adds a new API, path or config and every caller lives in this repo.

Rule. When no consumer sits outside the repo, move every caller to the new API and delete the old one, and the tests that pinned its internals, in the same change.

Why. Two supported ways to do one task both keep gaining callers. The old path becomes the one nobody dares remove.

+ def fetch_user_v2(id): ...
  def fetch_user(id): ...        # kept "for now"; 14 callers still use it

- def fetch_user(id): ...
+ def fetch_user(id): ...        # new behaviour; all 14 callers updated in this diff

Counterpoint. This rule applies only when no consumer sits outside the repo: a published library, a public API, a client on an older release, or data written by older code. For those, follow chg-expand-contract and remove the old form later. Check the consumer claim before you mark FAIL.

Tool gate: Dead-code and unused-export checks.

Sources: github.com, github.com, github.com, stripe.dev

Applies when: The change renames an environment variable, remaps a port, moves a secret source, or adds a step needed to run or build the project.

Rule. When a change breaks how developers or operators run the project, ship a migration note with it: old name, new name, and what to do.

Why. A renamed variable or moved secret fails on the next machine that pulls, often with a vague error far from the cause. The diff looks harmless because nothing in the code breaks.

- DATABASE_URL=...
+ DB_DSN=...            # no note, no fallback, .env.example untouched

+ DB_DSN=...            # reads DATABASE_URL with a deprecation warning for one release
+ CHANGELOG / PR text: "rename DATABASE_URL to DB_DSN; update .env and CI secrets"
+ .env.example updated

Counterpoint. "It is an internal repo; people will ask." Each person who asks costs more than the note, and CI and deploy environments cannot ask. For published configuration, chg-breaking-change-taxonomy has the wider rules.

Tool gate: Startup check that fails fast on missing or unknown config keys.

Sources: github.com

Applies when: A change fixes a bug, a failing test, or an incident.

Rule. In a bug fix, state the cause you saw in evidence, and reject a guard, retry, sleep, longer timeout, swallowed error, cast, skipped or weakened test, or special case for the failing input that the cause does not justify.

Why. Each of these hides the first wrong decision and leaves the bad state to surface somewhere else. The next caller hits the same fault, and the fix now needs a comment to explain why it works.

Bug: report total is None for some users.
Diff: if total is None: total = 0     # no cause given

Cause: the loader skips users with no orders (query output attached), so total is never set.
Diff: the loader returns an empty list for those users; the other two callers of the loader are checked.

Counterpoint. "A guard is the quick safe fix during an outage." Mitigate first if you must, but label it a mitigation and open the root-cause fix. A guard that ships as the fix is the FAIL.

Tool gate: Lint rules for as any, ts-ignore, .skip and empty catch.

Sources: github.com, github.com, github.com

Applies when: The change adds a feature flag, kill switch or config toggle.

Rule. Give every new flag an owner, a default, and a removal condition or date, and ticket the removal.

Why. A flag without an owner becomes a permanent fork of behaviour that nobody dares delete and that multiplies test paths.

if flags.get("new_checkout"):   # who owns it? when does it go?
    ...

# flag: new_checkout  owner: payments  default: off
# remove when 100% for 2 weeks  ticket: PAY-88
if flags.get("new_checkout"): ...

Counterpoint. Some flags are permanent (kill switches, entitlements). Label them as permanent, with an owner. The check is that the intent is explicit.

Tool gate: Flag registry linter that requires owner and expiry fields.

Sources: programmingisterrible.com, dropbox.tech

Applies when: The change links a ticket, spec or issue.

Rule. Trace every behaviour change in the diff to the stated requirement, and flag missing requirements and any change nobody asked for.

Why. Unrequested changes carry unreviewed risk and surprise the owners. As a heuristic, a misunderstood requirement does more damage than a typo, so check the diff against the ask first.

Ticket: "return 404 for deleted users".
Diff also changes pagination defaults and adds a retry wrapper.

Diff only changes the lookup path and its tests; the pagination idea
is filed as a separate ticket.

Counterpoint. "Obvious small fixes while I'm there are free." Sometimes, and Google wants reviewers to look at surrounding code too. Tiny same-file hygiene is fine if the description names it. Behaviour changes are not.

Sources: google.github.io, github.com

Applies when: A migration adds a column to a large PostgreSQL table.

Rule. Add the column nullable or with a constant default, then fill volatile, computed, or identity values in batches.

Why. A constant or NULL default is a metadata change. A volatile default, a stored generated column, or an identity column rewrites the whole table, and the default lock (ACCESS EXCLUSIVE) blocks every query on it for the duration.

ALTER TABLE events ADD COLUMN token uuid DEFAULT gen_random_uuid();   -- volatile: rewrites 400M rows

ALTER TABLE events ADD COLUMN token uuid;                 -- no rewrite
-- backfill in batches, then SET DEFAULT for new rows only

Counterpoint. "The table is small." Then the rewrite is cheap and the one-step form is fine. State the row count in the description. sys-migration-lock-timeout covers locking and timeouts.

Tool gate: Migration linter that flags volatile defaults, stored generated columns and identity columns on ADD COLUMN.

Sources: www.postgresql.org

Applies when: The change adds an endpoint, input parser, file upload, dependency, permission, secret or outbound call.

Rule. List any new attack surface in the description.

Why. A generalist reviewing a diff will not spot an exposed route the author never mentioned. A new input, trust boundary, credential, or dependency counts, and security-relevant changes go to a qualified reviewer.

PR "add debug endpoint /admin/dump". Description: "helper for support".
Auth not mentioned, merged by one general reviewer.

Description: "new endpoint, admin-only via role X, rate limited, no PII".
Security reviewer tagged. Randomness uses a crypto source.

Counterpoint. Naming threats on every PR is theatre. Only changes that add one of the listed surfaces need the section, and most PRs write "none".

Tool gate: SAST and dependency scanning in CI catch the mechanical part.

Sources: google.github.io, go.dev, cheatsheetseries.owasp.org, wiki.mozilla.org

Applies when: The change adds a public function, endpoint, option, interface or extension point.

Rule. Ship new API with at least one real caller in the same change, and fail speculative interfaces and options that nothing uses.

Why. Unused API has no proof it fits real needs, yet once published it must be kept. Interfaces designed before a consumer exists usually get the shape wrong. See also mod-yagni.

+ class StorageBackend(Protocol): ...      # one implementation, no second planned
+ def export(fmt="csv", compress=False, ...)  # nothing calls compress=True

+ export_csv(rows)   # called by the report job in the same PR

Counterpoint. Library authors must ship API ahead of users. Then the usage is an example or test in the change, and the surface stays minimal. The Go guidance (declare interfaces where consumed, return concrete types) points the same way.

Tool gate: Unused-export or dead-code detection in CI.

Sources: google.github.io, go.dev, programmingisterrible.com

Applies when: The change adds or rewrites a SQL query against a table that grows with users or time.

Rule. Attach the query plan for each new or rewritten query on a growing table, taken on production-sized data.

Why. A query that reads well can still scan every row. The plan shows a sequential scan or a missing index before the table grows into it, and the author is the person with the query and the data at hand.

+ SELECT * FROM orders WHERE customer_email = $1
  (description: "adds lookup by email"; no plan, no index)

+ CREATE INDEX orders_customer_email_idx ON orders (customer_email);
  EXPLAIN ANALYZE on a 40M-row copy: Index Scan using orders_customer_email_idx, 0.08 ms

Counterpoint. "The planner changes its mind as data grows, so a plan today proves little." It proves more than no plan. A plan from a tiny dev table does mislead, because small tables scan by design, so take it on data close to production size and say where it came from.

Sources: www.postgresql.org, dev.mysql.com, www.sqlite.org, docs.gitlab.com, aws.amazon.com

Applies when: The diff adds a suppression (lint ignore, type ignore, skipped test, swallowed error) or deletes a failing check.

Rule. Name the rule, the reason, and a ticket on every suppression; a swallowed error, skipped test, or disabled check is a FAIL unless justified.

Why. Silencing a diagnostic hides the problem it reports and removes the safety net for the next author.

x = load(cfg)  # type: ignore
try: sync()
except Exception: pass
@pytest.mark.skip   # flaky

x = load(cfg)  # type: ignore[arg-type]  # lib stub wrong, TICKET-42
try: sync()
except SyncTimeout as e: log.warning("sync timeout", exc_info=e); retry()

Counterpoint. Sometimes a warning is a false positive. Then suppress narrowly with a reason and an expiry date, which keeps the gate useful. A suppression of a rule that guards correctness or safety should be removed, not documented. Palantir's list covers TODOs without issue references and merging with checks red. The Go list covers discarded errors. This card extends both to suppressions.

Tool gate: Linter rule that requires a reason and ticket on each suppression.

Sources: go.dev, blog.palantir.com, github.com, github.com

Applies when: The change adds a conditional, flag or nullable mode to code that serves more than one feature.

Rule. Put a new case behind its own function or type; do not add a one-off branch to a shared flow.

Why. Each special case in a shared path is a condition that every later reader and every other feature must reason about. The cases accumulate until nobody can say what the path does.

def checkout(cart, user):
    ...
    if user.org == "acme":            # added for one customer
        total = total * 0.9

def checkout(cart, user, pricing=standard_pricing):
    total = pricing(cart, user)       # acme's rule lives in its own pricing function

Counterpoint. "It is one line; an abstraction is overkill." One is fine. The second and third special case are the signal. See mod-yagni for when not to extract yet. Temporary branches need an owner and a removal date (chg-flag-owner-retirement).

Sources: github.com

Applies when: The diff changes an existing field's type, default, label or name in a .proto file.

Rule. Never change the type of a protobuf field, switch it between repeated and scalar, or rename it while any consumer reads JSON or text format; add a new field instead.

Why. The wire format carries numbers and types, not names, so a type change is read as garbage by anything not yet redeployed. The JSON and text formats carry names, so a rename breaks those readers.

message Invoice {
-  int32 amount = 2;
+  string amount = 2;
}

message Invoice {
  int32 amount = 2 [deprecated = true];
  string amount_decimal = 3;
}

Counterpoint. "Both ends deploy together." A rolling deploy runs both versions at once, and stored data does not redeploy. If the message never leaves one binary, mark the card N/A.

Tool gate: buf breaking with the wire and JSON rule sets.

Sources: protobuf.dev

Applies when: The diff deletes a field or enum value from a .proto message or enum.

Rule. When you delete a protobuf field or enum value, add reserved for its number and name in the same change, and never reuse a number.

Why. Old data and old clients still carry the number. A new field that takes it is read as the old type, and the failure is silent corruption, not an error.

message Order {
-  string coupon = 4;
+  int32 loyalty_points = 4;   // number 4 reused
}

message Order {
  reserved 4;
  reserved "coupon";
  int32 loyalty_points = 5;
}

Counterpoint. "Nobody sends field 4 any more." Stored messages, queued messages and a client on last month's build do. Reserving costs one line and cannot be undone by accident.

Tool gate: buf breaking or an equivalent descriptor diff in CI.

Sources: protobuf.dev

Applies when: The change adds a helper, util, type or constant.

Rule. Search the repo for an existing helper before adding a new one.

Why. Reviewers see only the diff, so a reimplementation of a helper a few files away looks fine in isolation. Two copies then drift and get fixed once. The author names where they searched.

# utils/dates.py already has parse_iso(s)
def _parse_date(s):  # new, in billing/invoice.py
    return datetime.strptime(s, "%Y-%m-%d")   # drops timezone handling

from utils.dates import parse_iso

Counterpoint. "Searching costs more than writing ten lines." For tiny code that is true. Then the duplicate is tolerable and mod-yagni governs extraction. The cost flips once the logic holds a rule (parsing, money, auth) that must stay in one place.

Tool gate: Clone-detection in CI (jscpd, PMD CPD, or similar) can flag near-duplicate blocks.

Sources: blog.pragmaticengineer.com, dev.to, www.kernel.org, refactoring.guru

Applies when: A reviewer writes feedback on a change.

Rule. Label each review comment with its severity: block, should-fix, or discuss.

Why. Unlabelled comments leave the author unable to tell what blocks merge. Keep one finding per comment, so each label covers one thing and each thread can close on its own. Wording that blames the person stalls the discussion. Phrase comments as a question or a fact, not a verdict on the person.

"This whole function is bad, also naming, and your loop has a bug."

"[block] items[i+1] reads past the end when len==1. Repro: ..."
"[discuss] could this be named `retry_count`?"

Counterpoint. "Labels add ceremony to a tiny review." A label is one word, and it saves a round trip in which the author asks whether a comment blocks the merge. A team that marks only its nits still leaves the author to guess about every other comment, so label those too.

Sources: google.github.io, blog.pragmaticengineer.com, blog.palantir.com, slack.engineering, blog.cloudflare.com, dropbox.tech

Applies when: The change deploys schema, data, config or behaviour that is costly to undo.

Rule. State in the description how to undo the change.

Why. When a release goes wrong, the time to find the way back is before shipping. Irreversible steps (dropped data, sent messages) need an explicit decision. The author has checked that the route is possible. See inc-rollback-path-tested for drilling it and prod-canary-staged-rollout for abort rules.

Migration drops column legacy_id and deletes backing rows.
Description: no rollback section. Code and migration deploy together.

Rollback: revert PR; column kept for one release, dropped in a later PR.
Behaviour behind a flag; canary at 5% with an abort rule.

Counterpoint. "We can always roll forward." Fine for stateless code, but not after data is destroyed or a client has cached the new contract. The check is cheap for reversible changes (one line) and bites only on irreversible ones.

Tool gate: Migration linter requiring a down step; deploy pipeline with automatic abort.

Sources: sre.google, programmingisterrible.com

Applies when: The change alters markup, styles, or behaviour a user sees or operates.

Rule. A UI change shows before-and-after screenshots and says what a keyboard-only pass and a 200% zoom check found on the changed screens.

Why. A reviewer reading a diff cannot see focus, order, or overflow. A screenshot shows the visual change, and the keyboard pass shows what the diff hides: a lost focus ring, a trap, a control that cannot be reached.

PR: "Redesign the settings dialog"
Files: Settings.tsx, settings.css
(no screenshots, no mention of keyboard or zoom)

PR: "Redesign the settings dialog"
Before / after: screenshots attached (default and 200% zoom).
Keyboard: Tab reaches every control in order, Esc closes, focus returns to the gear button.
Not covered: screen reader, not tested.

Counterpoint. "Automated checks run in CI." They find a minority of problems. They cannot tell that Tab order is wrong or that focus vanished. Say what you did not test, as the last line shows.

Tool gate: A pull request template with a UI checklist; the screenshots and the keyboard result are the evidence.

Sources: www.w3.org, webaim.org, www.gov.uk, raw.githubusercontent.com, raw.githubusercontent.com, raw.githubusercontent.com