Reviewing AI code

Lesson 1 said the diff is where responsibility transfers to you. This lesson is what to actually do at that moment.

The core discipline is one sentence: review every diff, never blind-accept, and never skip human review on security-sensitive code. Everything below is either a reason that matters or a technique for doing it efficiently.

The failure modes AI reliably produces

Generic code review habits are tuned for human mistakes. Models fail differently, and consistently enough that you can hunt for specific things.

Three of these deserve more than a row.

Hallucinated packages

A model that does not know a library will invent a plausible one. The import looks right, the name sounds real, and npm install is one keystroke away.

// Looks entirely reasonable. Does this package exist?
import { parseISODuration } from 'iso8601-duration-utils';

Two reasons this matters more than an ordinary typo:

• It fails at install time if you are lucky, and at runtime if you are not. A hallucinated method on a real package compiles in a dynamically-typed language and blows up in production.
• Attackers register the names models hallucinate. If a model consistently invents iso8601-duration-utils, somebody will eventually publish something under that name. Installing an AI-suggested dependency without checking the registry is a supply-chain decision.

Verify every new dependency against the real registry, and be suspicious of any package you have not heard of that solves your problem suspiciously neatly.

Insecure defaults

Agents optimise for working, and the fastest way to make something work is to remove the thing stopping it.

Each of these is a correct solution to the problem as stated and a bad solution to the problem you have. They are also easy to skim past, because they are one line in a diff that is otherwise fine.

Removed safeguards

The sharpest one. Agents optimise to make code run, and will happily delete a validation or relax a policy to clear an error.

  export async function updateProfile(req, res) {
-   if (!req.user || req.user.id !== req.params.id) {
-     return res.status(403).json({ error: 'Forbidden' });
-   }
    const data = profileSchema.parse(req.body);
    await db.profiles.update(req.params.id, data);

An agent told "the profile update test is failing" may well produce exactly this. The test passes. The authorization check is gone.

This is why you read deletions as carefully as additions. Most review attention naturally goes to new code; the dangerous change here is a removal, and it is short.

Secrets

Two distinct problems that get lumped together.

// 1. Hardcoded — caught by most scanners, embarrassing but findable
const stripe = new Stripe('sk_live_51H8xK2...');
 
// 2. Logged — caught by almost nothing, and it reaches your log store
logger.info('outbound request', { url, headers });   // headers carry the token

The second is the one that survives review. A scanner looks for secret-shaped literals; nothing flags a perfectly ordinary log line that happens to serialise an object containing an Authorization header. When you see structured logging of a request, a response, or a config object, ask what is inside it.

Code inefficiency

Models produce code that is correct per-item and quadratic in aggregate, because each line looks reasonable in isolation.

// Correct. Also one query per order.
const orders = await db.orders.findMany({ where: { userId } });
for (const order of orders) {
  order.items = await db.items.findMany({ where: { orderId: order.id } });
}

The N+1 is invisible unless you are looking for it — the loop body is three words long and obviously right. The same blindness produces needless re-renders in React and the same helper function written three times in three files because each was generated in a separate turn.

Duplicated logic is the AI-specific variant. A human refactors on the second occurrence because they remember writing the first. An agent in a fresh context does not remember, so it writes it again.

Over-engineering

The opposite failure, and more common than under-delivery. Asked for a function, an agent may return an interface, an abstract base class, a factory, and a configuration object — all speculative, none requested.

Every speculative abstraction is code somebody maintains forever, justified by a requirement that does not exist yet and may never.

How to read an AI diff efficiently

Reading a large diff top-to-bottom is the slowest way to find the things most likely to be wrong. Order the passes by risk instead.

If a diff is too large to run these five passes on, that is not a reason to skip them. It is the signal to split the PR.

Git hygiene

Small atomic commits are worth more in an AI workflow than a manual one, for a reason that is easy to miss: they are how you bisect a branch you did not type.

The last row matters because Cursor's checkpoints are a within-session safety net. Commits are the version that is still there tomorrow.

AI cannot validate its own output

A model asked whether its code is correct will tell you it is. That is not dishonesty; it has no independent oracle. Pair it with things that do.

Notice the ordering: each layer is cheaper the earlier it sits. A rule that prevents a pattern costs nothing at review time; the same problem caught in CI costs a round trip; caught in production it costs an incident.

Before you accept a diff

The fourth item has a habit built into it. When you catch the agent breaking a convention, the fix is not only to correct this diff — it is to ask whether a rule should have prevented it. A convention you correct manually twice is a rule you should have written once.

Before you commit

The second item is Lesson 1's litmus test, arriving at the moment it becomes binding. A commit is a claim of authorship in the sense that matters: you are the person who will be asked about this code.

Keeping PRs small when the agent is fast

Agents make large PRs effortless to produce and no easier to review. The gap between those two facts is where review quality goes to die.

Practical habits that make stacking work:

• Commit atomically as you go. One coherent change per commit with a real message. This is what lets you split a branch later without archaeology.
• Let the loop decide the boundaries. Each pass through Lesson 1's loop — small step, gates, commit — is naturally one commit, and often one PR.
• Split before you push, not after review stalls. A 2,000-line PR that has sat for two days is harder to split than the same work was an hour after writing it.
• Sequence by dependency, not by file type. A stack where each PR merges cleanly on its own beats one where the third depends on a review comment in the first.

Data and privacy

Two controls, both worth setting deliberately rather than discovering later.

MCP is attack surface

Connecting Cursor to external tools through MCP gives an agent real capabilities — and gives a compromised or hostile server a path into your machine and your repository. Treat the configuration as security-relevant, because it is.

Use environment references rather than literals:

{
  "mcpServers": {
    "internal-api": {
      "command": "npx",
      "args": ["-y", "@acme/mcp-internal"],
      "env": { "API_TOKEN": "${env:ACME_READONLY_TOKEN}" }
    }
  }
}

The token name says READONLY for a reason. If the agent only needs to query, the credential it holds should only be able to query.

Recap

Lesson 5 puts all four lessons into the form an interviewer will ask for.