reviewing-changes

Reviewing a plan step's changes

You review code another agent wrote. Generated code fails differently from human code: it looks more correct than it is. Review the diff against the step's intent and evidence — never against "does it look plausible".

Order of review (where agent code actually fails)

1. Intent match first. Re-read the step title/note. The signature failure is a correct solution to a slightly different problem. Does the diff do what the step asked — all of it, and only it?
2. Scope drift. List the changed files before reading bodies. Anything beyond the step's ask (drive-by refactors, renames, new dependencies) that the summary did not disclose is a finding. Also scan every changed file for content that doesn't belong at all — a pasted URL/token/credential, leftover debug logging, commented-out code — it hides easily inside a file that's legitimately in scope, since nothing about the file itself looks wrong.
3. Verify the verification claim. Never trust "tests pass" — run the named check yourself when cheap (bun test <dir>, typecheck). CI gaming is a top failure: weakened/skipped/deleted tests, || true, broadened lint ignores, @ts-expect-error/as any — each is a finding even when the code is right.
4. Reality of every API. Hallucinated or wrong-signature imports/calls are common: check that each new import exists and matches the lockfile/docs, not the author's claim.
5. Correctness at the edges. Error paths, empty/None cases, concurrency, resource cleanup — agents under-test edges they didn't hit.
6. Hunt what is MISSING, not just what is wrong. The worst bugs in agent code are omissions — no added line is wrong, a needed line is absent, so reading the diff top-to-bottom finds nothing. For every piece of state the change introduces or touches (in-memory map/latch/registration/mark, temp resource, persisted flag), trace its full lifecycle in the resulting code: where it is set → where it is cleared → what clears it on EVERY exit path (success, error, abort, a later unrelated turn). A set without a clear on some path is a finding. For every fire-and-forget or detached call, name what rolls back when it rejects. Then check the OTHER side: for every READER of that same state, ask what it does when the state is absent or stale. A permissive default at the read site (an optional spread, ?? fallback, silently continuing instead of rejecting) is a finding exactly like a missing clear — a clean producer-side lifecycle still lets a consumer misbehave on the gap.
7. Invariant audit. Collect the explicit guarantees in the owning module's SPEC.md (and its parent) that touch the changed seams, then check each changed code path against each guarantee — mechanically, path × guarantee (spec_grep the area). A diff that contradicts a recorded decision or silently drops a stated guarantee is a finding even if it works. The reverse gap is a finding too: a diff that establishes or changes an invariant without updating the owning SPEC.md to state it — the next review reads a spec that no longer describes reality.
8. Parallel surfaces and external defaults. When two paths produce the same artifact (primary + fallback, manual + automated, UI + wire), diff their inputs — one deriving from state the other ignores is a finding. For every external command/API call, name the defaults the code silently relies on (target branch, cwd, locale, config lookup) and verify each is the intended one. When a config documents a state ("unset ⇒ default"), verify the UI/wire can actually reach it — a documented state with no transition into it is a finding.
9. Style is NOT your job unless it hides a bug.