How to review AI generated code

An adversarial review workflow

1. Freeze the contract before trusting the implementation

Translate the request into observable outcomes: inputs, outputs, authorization, error behavior, data changes, and rollback. State the smallest allowed file set. This makes a useful question out of every extra line: which acceptance condition requires it?

# Start with the boundary, not the generated explanation.
git diff --name-status origin/main...HEAD
git diff --check
git diff -- src/routes/orders.ts test/orders.test.ts

# Search for callers and old behavior before accepting a replacement.
rg -n "createOrder|legacyCreateOrder|ORDER_TIMEOUT" src test docs config

2. Trace values across boundaries

Follow every new or changed value from source to sink. Inputs may come from HTTP, queues, environment variables, files, databases, or another service. Sinks include queries, shell commands, paths, templates, logs, caches, feature flags, and migrations. At each boundary ask what validation, encoding, timeout, ownership, and error translation the system expects.

3. Try to disprove the happy path

Use a small counterexample instead of a vague request for more tests: remove the optional field, make the dependency return 503, send a duplicate event, deny the caller, hold a database lock, or run the documented command from a clean checkout. The goal is not to create random failures. It is to test the exact assumption the diff makes at a system boundary.

4. Re-read the final diff after verification

Generated patches frequently include an unnecessary helper, dependency bump, formatting sweep, or changed default alongside the requested work. Re-open the final diff after tools run; do not let a successful command erase the question of whether the change is minimal and reviewable.

Write findings so another engineer can reproduce them

A review comment should contain a falsifiable claim, not a prediction that “this may be unsafe.” Use the compact SEVERITY / LOCATION / FAILURE / REPRO format. It forces the reviewer to name the impact, exact code, broken contract, and minimum proof.

SEVERITY: High — authenticated users can read another tenant's order.
LOCATION: src/routes/orders.ts:48, GET /orders/:id
FAILURE: The lookup filters by id but not accountId from the session.
REPRO: Create an order in account B; call the route as account A with B's id;
       the response returns 200 and B's order.

Severity describes the consequence if the reproduction succeeds, not how much you dislike the code. Location should survive a small rebase: include a symbol, route, migration, or command as well as a line when possible. Failure names the violated contract. Repro gives deterministic setup, action, and expected versus actual result. If you cannot supply a repro, label the concern as a question or investigation rather than presenting it as a defect.

Eight failure families to test

1. Happy-path-only validation

A generated handler often validates that a field exists but not that it belongs to the caller or has the required state.

// Incorrect: the id exists, but may belong to another account.
const order = await db.order.findUnique({ where: { id: req.params.id } });

// Review target: scope the lookup to the authenticated account as well.

2. Tests that cannot fail meaningfully

Test names can sound precise while assertions accept the broken behavior. Make the test fail by mutating the implementation.

// Too broad: 200, 401, and 404 all pass.
expect(response.status).toBeLessThan(500);

// Contract-level assertion:
expect(response.status).toBe(401);

3. Error swallowing

A fallback can turn an outage into a valid empty result and hide the signal needed by callers and operators.

try {
  return await inventory.get(sku);
} catch {
  return { available: false }; // outage now looks like a stock decision
}

4. Injection through a newly convenient interpreter

Concatenating an input into a command, query, path, or template adds an interpreter boundary that the happy path does not reveal.

// Avoid a shell and validate the expected identifier first.
execFile("git", ["show", "--", commitId], callback);

5. Authorization or tenant-boundary loss

Refactors can move an authorization check after a fetch or drop a scope filter when replacing a repository call.

// Risky: fetches any document before checking ownership.
const document = await documents.findById(id);
if (document.ownerId !== session.userId) throw forbidden();

6. Data and concurrency assumptions

Code that reads, checks, and writes in separate operations can duplicate work when two requests arrive together.

if (!(await hasProcessed(event.id))) {
  await chargeCard(event);      // concurrent workers can both reach this line
  await markProcessed(event.id);
}

7. Scope creep and dependency drift

A small requested fix can arrive with generated files, package upgrades, renamed helpers, and unrelated formatting. That weakens review even if every individual edit looks reasonable.

git diff --stat origin/main...HEAD
git diff -- package.json package-lock.json
git diff --name-only origin/main...HEAD

8. Documentation and operational drift

An AI patch may change a flag or environment variable without changing the command a deployer actually copies.

rg -n "OLD_TIMEOUT|REQUEST_TIMEOUT_SECONDS" README.md docs examples src test
node ./bin/service.js --help > /tmp/current-help.txt
diff -u docs/cli-help.txt /tmp/current-help.txt

These are families, not a replacement for domain knowledge. Add domain-specific attacks for money movement, privacy, retention, correctness proofs, hardware, and regulated workflows. The right review test is the smallest one that could invalidate the patch's central claim.

When to reject the whole diff

Reject rather than line-edit when the patch has no stable contract, changes too many unrelated surfaces to establish intent, or cannot be tested safely in the available environment. Also reject when it introduces a security or data-integrity risk with no credible mitigation, changes generated or lock files without a necessary source change, or depends on an unexplained framework behavior. A patch that passes today but cannot be understood or rolled back by the team is not ready merely because it compiles.

A whole-diff rejection is not a verdict on the author or model. It is a request for a smaller, independently reviewable change: restate the acceptance test, isolate the required files, add a negative test, and show the precise verification command. If the model cannot explain the boundary in code and evidence, start again from the contract rather than accumulating repairs.

Use a two-model second opinion without outsourcing judgment

A second model is useful when it has a different task and sees the evidence, not when it is asked whether the first model is “correct.” Give reviewer two the request, changed-file list, diff, relevant interfaces, and the tests already run. Ask it to produce only reproducible findings in the same four-part format, then independently run the strongest reproductions yourself.

Review this diff adversarially. Do not summarize it.
For each finding, return only:
SEVERITY / LOCATION / FAILURE / REPRO.
Prioritize authorization, data loss, concurrency, error semantics,
untrusted input, and tests that would pass if the implementation were broken.
If there is no reproducible finding, say so and list the highest-risk
assumptions that still need a human or integration test.

Keep the models independent: do not feed the first model's conclusion to the second before it reviews the diff. Agreement is not proof, and disagreement is not a tie-breaker. It is a way to generate focused hypotheses. The accountable reviewer still decides whether the contract is met and whether the evidence is strong enough to ship.