← Review Kit

Worked example: reviewing an agent-generated PR

Synthetic teaching scenario. Plausible code you might merge on a skim. That is the point.

Setup

I asked Cursor to add PATCH /api/widgets/:id for admin users — update a widget label, return JSON, 404 if missing. Small task. PR landed Friday. Diff looked tidy. Tests green. I almost merged before dinner.

I ran the checklist instead. About twelve minutes. Glad I did.

Files changed: routes/widgets.ts, middleware/auth.ts, tests/widgets.test.ts, config/defaults.ts

The diff (abbreviated)

routes/widgets.ts — looks fine at first glance

export async function patchWidget(req, res) {
  const { id } = req.params;
  const { label } = req.body;
  if (!label || label.trim() === "") {
    return res.status(400).json({ error: "label required" });
  }
  const widget = await db.widgets.update(id, { label: label.trim() });
  if (!widget) return res.status(404).json({ error: "not found" });
  return res.json(widget);
}

middleware/auth.ts — scope drift in a "refactor"

// Before: adminOnly — role === 'admin'
// After: requireRole(['admin', 'editor'])
// Comment: "Refactored auth helper for flexibility."

I did not ask for editors. Admin only.

tests/widgets.test.ts — green but hollow

it("updates widget label", async () => {
  const res = await request(app)
    .patch("/api/widgets/w1")
    .send({ label: "New Label" });
  expect(res.status).toBe(200);
});

No auth header. No 404. No empty-label case.

config/defaults.ts — unrelated

export const WIDGET_CACHE_TTL_SECONDS = 300; // was 60

Nothing in my prompt mentioned caching.

Checklist pass

1. Scope drift — FLAG

Editors allowed in auth middleware — not in spec. Cache TTL changed — unrelated. The route file alone would have been enough.

Would have missed: Permission widening is the highest-risk change. It is not in the route file I skimmed first.

2. Behavior & edge cases — FLAG

Empty label rejected — good. No tests for non-admin callers, malformed JSON, or length limits. Happy path only.

4. Test reality — FLAG

Single test without auth — may pass even if middleware were removed. Would not fail if empty-label validation were deleted. Performative.

6. Agent self-review

Agent flagged editor scope expansion, missing auth tests, and unrelated config — same items I found.

Use the full checklist

This is one scenario. The reusable pass:

Synthetic scenario for teaching. Feedback welcome.