2026-07-22 · 6 min read

A code review example: one PR, fully annotated

Search for a code review example and you mostly get definitions: what code review is, why it's good, a numbered process. What you rarely get is an actual review. So here's one, end to end: a small AI-written PR, the comments a strong reviewer leaves on it, and the verdict with reasons. Read the diff first and form your own opinion before you read mine.

The PR

The task given to the AI assistant, as three requirements:

Add GET /invoices/export returning the requester's invoices as CSV.
Accept from and to date query params to bound the range.
Columns: id, customer name, total.

And the diff it opened. Auth middleware already runs on this router, so req.user is populated:

ai-agent wants to merge · routes/invoices.js +11

1+ router.get('/invoices/export', async (req, res) => {

2+ const { from, to } = req.query;

3+ const invoices = await db.invoices.findBetween(from, to);

4+ const rows = invoices.map(

5+ (i) => `${i.id},${i.customerName},${i.total}`

6+ );

7+ const csv = 'id,customer,total\n' + rows.join('\n');

8+ res.setHeader('Content-Type', 'text/csv');

9+ res.setHeader('Content-Disposition', 'attachment; filename=invoices.csv');

10+ res.send(csv);

11+ });

Eleven lines, reads clean, does roughly what was asked. This is exactly the kind of diff that gets approved between two meetings. Now the review.

The review, comment by comment

Comment 1, line 3: the missing tenant scope

2+ const { from, to } = req.query;

3+ const invoices = await db.invoices.findBetween(from, to);

Yyou commented

This queries every invoice in the range, not the requester's. The requirement says the requester's invoices, and nothing here filters by req.user. As written, any signed-in user exports the whole company's billing.

4+ const rows = invoices.map(

The most severe finding, and notice what makes the comment work: it doesn't say "add a filter", it names the blast radius. Missing ownership checks are among the most common holes in AI-written endpoints (broken object level authorization has topped the OWASP API Security Top 10 for years), because the model implements the sentence it saw ("export invoices between dates") and quietly drops the qualifier it didn't ("the requester's"). Requirement-vs-implementation reading catches what type checkers never will.

Comment 2, line 5: the CSV that executes

4+ const rows = invoices.map(

5+ (i) => `${i.id},${i.customerName},${i.total}`

Yyou commented

customerName goes into the CSV unescaped. A name containing a comma breaks the columns, and a name starting with = becomes a live formula the moment someone opens this export in Excel. Quote the field and prefix formula triggers, or build the file with a csv library instead of string concat.

6+ );

Confession: we shipped this exact bug in DiffDojo's own admin export, and our pre-launch security audit caught it, not a reviewer. String-concatenated CSV is a classic (OWASP documents the attack) because it works perfectly on every demo dataset. The trained reflex here is simple: user-controlled text crossing into another format (CSV, HTML, SQL, shell) always gets an escaping question.

Comment 3, line 2: the parameters nobody validated

1+ router.get('/invoices/export', async (req, res) => {

2+ const { from, to } = req.query;

Yyou commented

What happens when from or to is missing or not a date? If findBetween(undefined, undefined) returns everything, this endpoint dumps the full table by default. Worth validating both and failing with a 400.

3+ const invoices = await db.invoices.findBetween(from, to);

Phrased as a question, and that's deliberate. The reviewer doesn't know what findBetween does with undefined, and the comment says so honestly. A question that forces the author to check is a completely valid review move; pretending to certainty you don't have is not.

What didn't get a comment

The template-string formatting, the header names, the inline handler instead of a named function: all fine. A weak review of this PR looks like the opposite of the one above: two style nitpicks about naming, an emoji, and an approve. Restraint on the small stuff is what buys your serious comments their weight.

Anatomy of a strong review comment, shown on the CSV injection comment: the observation (specific and anchored to a line), why it matters (the concrete consequence), and a way out (a fix or a question).

The verdict

Request changes, and the summary comment ties it together:

Requesting changes for the tenant scoping on line 3, which I'd consider a blocker. The CSV escaping and param validation should land with it. Everything else looks good, and the shape of the endpoint is right.

Note the shape: the blocker named first, the severity made explicit, the good parts acknowledged without ceremony. An approve here, with a "maybe add validation later", would have shipped a company-wide data leak that no test suite was going to catch, because every test would use well-formed dates and its own tenant's data.

The pattern behind the comments

Every comment above has the same three parts: the observation, anchored to a line; the concrete consequence; and a way out, either a fix or an honest question. Every one of them also came from a repeatable habit rather than brilliance:

Reread the requirements after the diff, and check each qualifier survived into the code (comment 1).
Follow user-controlled data across format boundaries (comment 2).
Ask what the ugliest legal input does (comment 3).
Say nothing about what's merely different from how you'd write it.

Habits become reflexes through reps, and reviewing is no exception. That's the premise DiffDojo is built on: one realistic AI-written PR like this one every day, your comments and verdict graded against a canonical review. The daily PR is free, no signup. Today's has a different bug in it.

You've seen the annotated answer. Now try one cold: fresh PR, no annotations, your call.

Review today's PR, free