ComputeSphereLearn
Console
Learning paths / Vibe coding to production / Read every diff

Review an agent's pull request

Reading · 8 min · Module 4, lesson 2 of 313 min left in this module

Module 4 · Read every diffLesson 2 of 3

Goal: Review a real agent-style change to the starter with the diff checklist, and find the security bug its tests don't catch.

4:01 · captions and chapters · narrated with an AI-generated voice
Transcript

Narration uses an AI-generated voice.

[00:00] Where we're going

By the end of this video, you'll be able to review an agent's pull request, and find the bug its tests don't catch. Because a change can be tidy, tested and well described, and still be unsafe.

[00:13] The change

Here's the change. It adds a CSV export, so you can download every task as a spreadsheet file. One commit, two files. The server, and a new test file. It comes with tests, and they pass. Start with what it claims. The commit message says cells with commas, quotes or newlines are quoted. You'll check the code against that.

Then go through the checklist from the last lesson. Scope. Dependencies. Input. Who may do this. Secrets. Weakened checks. And the criteria. Spend longest on input. Where does each cell's text come from, and where does it end up?

[00:56] Read the diff

Here's the new function that builds the CSV. For each cell, it checks for a comma, a quote or a newline. If it finds one, it wraps the cell in quotes. That matches the description. Then the route. It puts every task's title into the file. And those titles come from any user.

[01:17] Where the input ends up

So follow one title. Say someone adds a task called equals one plus one. No comma, no quote, so it goes into the file exactly as typed. Now open that file in a spreadsheet. To a spreadsheet, a cell that starts with equals, plus, minus or the at sign is a formula. So this cell shows two. A crafted title can build a link that sends other cells' data elsewhere when clicked, on the machine of whoever opens the export. That's CSV injection.

[01:53] Why the tests pass

So why do the tests pass? Look at what they check. Commas, quotes and newlines. The escaping the author thought of. No test tries a title that starts with equals. Tests only prove what someone thought to test.

[02:10] Two more comments

The checklist finds two more things. Scope. To make the app testable, the change rewrites the listen lines you fixed in lesson four point two point three. The new lines still use the old, local-only address. Merged into your fork, that conflicts. Resolve it carelessly, and the old bug comes back. And who may do this. The export includes every user's tasks. The task list already does that, so it's not new. Note it for module six.

[02:44] Write the review

Now write the review. A useful comment names the line, the risk, and the fix. Something like this. Titles starting with equals export as formulas. Prefix those cells with an apostrophe, and add a test for it. An agent can act on that directly.

The fix puts an apostrophe in front of any cell starting with one of those characters, or a tab or carriage return, then quotes as before. And a new test checks that equals one plus one comes out with the apostrophe in front.

[03:21] Recap

To recap. Read what the change claims, then check the code against it. Follow every input to where it ends up. Passing tests only prove what someone thought to test. So review against the checklist, not against how confident the change looks.

Here's a question to check yourself. The tests pass and cover quoting. Why didn't they catch the formula problem? Because no test tried a cell starting with equals, plus, minus or the at sign. Next, check what you've learned. I'll see you there.

Key idea

A change can be tidy, tested and described well, and still be unsafe. Review it against the checklist, not against how confident it looks. Here you'll do that on a change an agent could easily have written.

The change adds a CSV export to the starter: GET /api/tasks/export.csv downloads every task as a spreadsheet file. It comes with tests, and they pass.

  1. Open the change

    Open the comparison on GitHub and go to Files changed. It's the same view as a pull request.

    You should seeOne commit, two files changed: server.js and a new test/export.test.js.

  2. Read what it claims

    Read the commit message first. It's what the author, human or agent, says the change does. You'll check the code against it.

    You should seeA description saying cells with commas, quotes or newlines are quoted.

  3. Go through the checklist

    Scope, dependencies, input, who may do this, secrets, weakened checks, criteria. Spend longest on input: where does each cell's text come from, and where does it end up?

    You should seeNotes against each of the seven questions from the last lesson.

  4. Try it yourself (optional)

    In your clone, fetch the branch and run it:

    git fetch https://github.com/computesphere-samples/learn.git vibe-starter/agent-export
git switch -c agent-export FETCH_HEAD
npm install && npm start

    In a second terminal, add a task titled =1+1 and export:

    curl -X POST localhost:3000/api/tasks -H "Content-Type: application/json" -d '{"title":"=1+1"}'
curl localhost:3000/api/tasks/export.csv

    To see it as a user would, open http://localhost:3000/api/tasks/export.csv in your browser, open the downloaded tasks.csv in a spreadsheet, and look at that cell. git switch main takes you back.

    You should seeA line in the CSV reading 4,=1+1,,false,demo,false.

Decide what you'd write in the review before you open the answer.

What the review should find

The bug: CSV injection. Titles come from any user and go into the file as typed. toCsv quotes commas, quotes and newlines, but a spreadsheet treats a cell starting with =, +, - or @ as a formula. Your =1+1 shows as 2. A crafted title can build a link that sends other cells' data elsewhere when clicked, on the machine of whoever opens the export.

The fix is to put a ' in front of any cell starting with one of those characters (tab and carriage return too), then quote as before. Add a test that a title of =1+1 comes out as '=1+1.

Why the tests pass. They check the escaping the author thought of. Tests only prove what someone thought to test.

Also worth a comment:

  • Who may do this: it exports every user's tasks. So does GET /api/tasks today, so it's not new, but note it for module 6.
  • Scope: to make the app testable, it rewrites the app.listen lines you fixed in lesson 4.2.3, still on 127.0.0.1. Merged into your fork, that conflicts, and resolving it carelessly brings the old bug back.
  • Dependencies: none added. Good.

Why this one is easy to miss

Nothing about the change looks careless. It handles the quoting cases most CSV examples handle, it has tests, and its description is accurate. That's typical of agent-written code: it follows the common pattern well, and the common pattern is missing a case.

The checklist is what finds it. "Where does this input end up?" leads you to a spreadsheet, and from there to the question of what a spreadsheet does with a cell. Confidence and tidiness tell you nothing either way.

Write the review

A useful review comment names the line, the risk and the fix, for example: "Titles starting with = export as formulas. Prefix those cells with ' and add a test for it." An agent can act on that directly. "Looks good" teaches nobody anything.

Check yourself

The export's tests pass and cover quoting. Why didn't they catch the formula problem?
To make the app testable, the change also rewrites the app.listen lines, still on 127.0.0.1. What's the risk for your fork?