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 with arrows between boxes. What you almost never get is someone actually reviewing a piece of code in front of you, comment by comment, and explaining why they wrote what they wrote and left alone what they left alone. So here's one, end to end: a small AI-written PR with three planted problems, 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, because that's the whole point of this exercise.

The PR

The task given to the AI assistant was straightforward: add GET /invoices/export returning the requester's invoices as CSV, accepting from and to date query params to bound the range, with columns for id, customer name, and total. Auth middleware already runs on this router, so req.user is populated. Here's the diff it opened:

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(

This is the most severe finding in the diff, and it's worth pausing on what makes the comment work: it doesn't just say "add a filter," it names the blast radius, which is that any signed-in user can download the entire company's billing history. Missing ownership checks are among the most common holes in AI-written endpoints, and broken object level authorization has topped the OWASP API Security Top 10 for years for good reason. The model implements the sentence it saw ("export invoices between dates") and quietly drops the qualifier it didn't ("the requester's"), which is exactly the kind of fidelity bug that survives every automated check because the code does exactly what the code does, just not what the spec says.

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 it was our pre-launch security audit that caught it, not a reviewer. String-concatenated CSV is a classic attack vector (OWASP documents it) and it's a classic precisely because it works flawlessly on every demo dataset anyone tests with, since the demo data never has a customer named =cmd|'/C calc'!A0. The trained reflex here is the same one that catches SQL injection and XSS: any time user-controlled text crosses into another format, whether that's CSV, HTML, SQL, or a shell command, you ask the escaping question before you approve.

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);

Notice that this one is phrased as a question, and that's deliberate. The reviewer genuinely doesn't know what findBetween does when you hand it undefined for both bounds, and the comment says so honestly instead of pretending to certainty it doesn't have. A question that forces the author to go check is a completely valid review move, and often a better one than a confident assertion that turns out to be wrong about the library's internals.

What didn't get a comment

The template-string formatting, the header names, the fact that it's an inline handler instead of a named function: none of that got a comment, because none of it is a problem. This is worth calling out because a weak review of this same PR looks like the exact opposite of the one above: two style nitpicks about variable naming, a thumbs-up emoji, and an approve. The reviewer who spent their comment budget on "consider renaming i to invoice" has no weight left for the comment that would have stopped a company-wide data leak. Restraint on the small stuff is what buys your serious comments their authority.

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.

Look at the shape of that verdict: the blocker is named first so nobody has to hunt for it, the severity is made explicit so there's no ambiguity about whether this is a suggestion or a stop-ship, and the good parts are acknowledged without ceremony so the author knows this isn't a pile-on. An approve here, with a "maybe add validation later" tacked on as a non-blocking suggestion, would have shipped a company-wide data leak that no test suite was ever going to catch, because every test anyone would write for this endpoint would use well-formed dates and query its own tenant's data.

The pattern behind the comments

If you go back and read all three comments, you'll notice they share the same three-part structure: an observation anchored to a specific line, the concrete consequence of leaving it as-is, and a way out that's either a fix or an honest question. And every one of them came from a repeatable habit rather than a flash of brilliance. Comment 1 happened because the reviewer reread the requirements after the diff and checked whether each qualifier actually survived into the code. Comment 2 happened because the reviewer followed user-controlled data across a format boundary. Comment 3 happened because the reviewer asked what the ugliest legal input would do. And the discipline of not commenting on the template-string formatting or the inline handler happened because the reviewer understood that saying nothing about code that's merely different from how you'd write it is what buys your serious comments their weight.

All of these are habits, and habits become reflexes through reps, and reviewing is no exception to that. That's the premise DiffDojo is built on: one realistic AI-written PR like this one every day, where you leave your comments and your verdict and then get graded against a canonical review that shows you exactly what you caught and what you walked past. The daily PR is free, no signup, and today's has a different bug in it.

Try a review

You've seen the annotated answer. Now try one cold.

Fresh PR, no annotations, your call. Free, no signup. You get the canonical review after your verdict.

Review today's PR →