Reviewing What the Agent Built
Week 8 · Review the change, not the diff
WEEK 8Friday
WEEK 8Friday
Approved, merged, shipped
Randall Munroe, xkcd 1354, CC BY-NC 2.5, rearranged.
Key point
A diff shows what was added, never what was left out.
WEEK 8Friday
What review actually finds
Expected
Finding defects.
Delivered
Fewer defects than expected. Knowledge of code you did not write. Findings that are about 75% style and structure.
WEEK 8Friday
The rubber stamp has a price
- Low review participation: up to 5 more post-release defects per component.
- Past about 400 lines in a session, review stops finding things.
- With an AI assistant: less secure code, more confidence that it was secure.
Ask a programmer to review 10 lines of code and they will find 10 issues. Ask them to review 500 lines and they will say it looks good.
WEEK 8Friday
Every layer has holes
Key point
Your slice is the only one holding the use case.
WEEK 8Friday
Two questions every review answers
- Does this change do what the contract says?
- How deep must I read this part, given where it lands?
WEEK 8Friday
Open the test list beside the diff
| Row | Asserts | Test? |
|---|---|---|
| UC-NOT main, WAR | Only students with no activity are emailed | |
| UC-NOT 4a | Inactive week: 400, no email |
|
| UC-NOT 6a | One send fails: the others still go | |
| UC-NOT authorization | Unassigned instructor, student: 403 |
Ask the room
A missing extension: where would you see it, in the diff or here?
WEEK 8Friday
How deep
| Commit | Read |
|---|---|
| Trunk | Every line |
| Security rule or migration | Every line, and where it sits |
| Leaf | Its tests against the test list; skim the rest |
WEEK 8Friday
One word
// The design:
.requestMatchers(POST, base + "/sections/{sectionId}/reminders")
.access(anyOf(sectionInstructor, sectionOwnership))
// The quickest way to make a 403 test pass:
.requestMatchers(POST, base + "/sections/{sectionId}/reminders")
.authenticated()Pitfall
Any logged-in student can now email an entire course section.
WEEK 8Friday
Write the triage down
Trunk: SubmissionStatusService, read in full.
Security: the two route rules, read in full.
Leaves: controller and dialog; tests checked against rows 1 to 11; skimmed the rest.Key point
A skim you wrote down is a decision. A skim you did not is a rubber stamp.
WEEK 8Friday
Checklists save lives
Key point
Five steps for central lines. Infections: 11% to zero.
WEEK 8Friday
The review checklist
- Each test-list row: covered, or named as missing.
- Trunk and security commits read in full.
- The logic matches the description and the use case.
- Each extension's error path, and its log line.
- Tests check the contract, not a mock's echo.
WEEK 8Friday
Label every comment
issue (blocking): test-list row "UC-NOT 4a" has no test.
What does POST /reminders do for an inactive week?
nitpick (non-blocking): `cnt` → `sentCount`.WEEK 8Friday
When to approve
Key point
Approve when the change is better than what was there, not when it is perfect.
"nit: missing period in comment." On a pull request that drops the users table.
WEEK 8Friday
The author's half
| Section | Carries |
|---|---|
| Why | The problem, readable without the issue |
| What changed | A summary; decisions the code does not show |
| How it was verified | Verified: tests mapped to rows. Not verified: what nobody checked |
| Reviewer focus | The trunk commit, any security rule, where to start |
The agent
"Agents write convincing looking pull request descriptions. You need to review these too!" (Simon Willison)
WEEK 8Friday
The most useful line
Not verified: the email failure path.Key point
It tells the reviewer where to look. It is never penalized.
WEEK 8Friday
Your turn: the seeded pull request
Ask the room
Find what matters. Label each comment. Do not block on nits.
WEEK 8Friday
The agent as first reviewer
- Fresh context: it never saw the conversation that wrote the code.
/code-review 42;--commentposts its findings.- It reviews the diff, not the change, unless you give it the contract.
The reviewer bot approved its own pull request. It found the code clear, well structured, and written by someone with excellent taste.
WEEK 8Friday
Same pull request, run twice
| Run | Found | Missed |
|---|---|---|
/code-review <PR> |
||
| With the use case and test list |
WEEK 8Friday
Triage its findings
- Each finding: fixed, or rejected with a reason.
- Spot-check one it flagged and one it passed.
- No mention of the test list? It reviewed the diff.
WEEK 8Friday
Before your slice merges
- A teammate who did not write it reviews it.
- The triage line, and every test-list row marked.
- Labeled comments.
- One
/code-reviewrun with the contract, each finding answered.