Use when reviewing a single historical commit, patch file, or pasted diff.
Check:
what the commit changes
whether the change is internally complete
whether follow-up commits are needed to understand the final state
whether the patch applies assumptions not visible in the provided context
Agent-Generated Change Review
Use this mode when an AI agent produced or substantially edited the change.
Be extra suspicious of:
tests removed, skipped, weakened, or rewritten to match the new behavior without proving it
CI steps made conditional or non-blocking
copied patterns that ignore a better existing helper
duplicated validators, clients, hooks, schemas, or business rules
confident-looking code that misses product context, incident history, or operational constraints
broad rewrites that hide a small intended change
generated docs that claim behavior was verified when it was not
The code looking clean is not proof that the behavior is correct.
Review Passes
Use judgment. Do not mechanically force every pass when the change is tiny, but do not skip a relevant pass because the diff is pleasant to read.
1. Intent and Scope
What is the change trying to accomplish?
Does the implementation match that intent?
Is this change appropriate for this codebase now?
Is unrelated churn mixed in?
Are generated, vendored, lockfile, or formatting changes hiding functional changes?
2. Functionality
Does the code do what users, callers, or operators need?
What edge cases changed?
What happens for empty, missing, invalid, duplicated, stale, concurrent, or unauthorized inputs?
Does the change preserve backward compatibility where required?
Are errors surfaced, logged, or handled at the right boundary?
3. System Fit
Does the change use existing local patterns, helpers, types, schemas, services, and conventions?
Is new abstraction justified by real complexity?
Is duplicated logic better replaced by an existing module?
Are boundaries between UI, API, persistence, auth, CLI, and background work preserved?
4. Security and Privacy
Check this pass whenever the change touches user data, private data, files, auth, sessions, roles, tokens, credentials, billing, webhooks, external APIs, uploads, downloads, logs, or admin flows.
Is identity derived from trusted server context rather than user input?
Are ownership and authorization enforced server-side?
Could private data leak through public routes, logs, errors, generated files, or caches?
Are secrets read by name only and never printed or committed?
Do new external calls have safe inputs, timeouts, and failure handling?
5. Data and Migrations
Use when data shapes, persistence, schemas, migrations, serialization, or backfills changed.
Is the migration deployable with the current app order?
Is rollback or forward-only behavior understood?
Are old rows, nulls, duplicate rows, and partially migrated states handled?
Are constraints, indexes, defaults, and ownership fields correct?
Does serialized data stay compatible with existing clients or stored records?
6. Tests and Verification
Are tests added or updated for changed behavior?
Would the tests fail on the bug they claim to cover?
Are assertions meaningful rather than snapshots of implementation accidents?
Is the right layer tested: unit, integration, end-to-end, CLI, browser, API, migration, or manual smoke?
Did CI, lint, typecheck, coverage, or required checks get weakened?
Tests are code too. Do not accept complex or brittle tests just because they are not production code.
7. Operations and Release Risk
Use when the change can affect deploys, runtime behavior, support, observability, performance, or rollback.
Are environment variables, flags, jobs, queues, caches, cron, or infrastructure touched?
Are logs and errors useful without exposing sensitive data?
Are smoke checks or runbooks needed?
Could the change fail only in production scale, slow networks, old clients, or cold starts?
8. Documentation
If the change affects how users or developers build, test, configure, run, deploy, call, or operate the system, check that relevant docs changed too.
Do not demand docs for every internal cleanup. Do ask for docs when the change creates a new workflow, command, API contract, environment variable, migration step, or operational rule.
Finding Bar
Only report a finding when it is:
based on changed code or directly affected unchanged code
concrete and reproducible from the available evidence
likely to matter to users, developers, operators, security, data integrity, or maintainability
specific enough to fix
not merely a preference
Avoid speculative findings. If something is plausible but unproven, put it under "Questions" or "Residual Risk" instead of presenting it as a bug.
Severity
P0: release-blocking or production-breaking issue, severe data/security incident, or destructive failure
P1: high-impact bug, security/privacy issue, data loss risk, broken core workflow, or deploy hazard that should be fixed before merge
P2: real defect or maintainability risk that should be fixed soon but may not block urgent progress
P3: minor issue, local cleanup, low-risk consistency problem, or optional improvement
Nit: optional polish that should not block approval
Output Format
If findings exist, list them first in descending severity.
Use this shape for each finding:
P1 path/to/file.ts:42 - Short imperative title
Explain the concrete problem, the scenario where it fails, and why it matters. Keep it brief. Suggest the smallest safe fix when clear.
Then include:
Questions only for blockers or uncertainty that changes the review outcome
Verification listing checks actually run or evidence inspected
Residual risk naming important untested or unseen surfaces
Summary in 1-3 sentences
If there are no findings, say so directly:
No findings.
Verification: ...
Residual risk: ...
Do not bury a real finding under a general summary. Do not open with praise. Do not include long code rewrites unless the user asked for fixes.
Comment Style
Be direct, calm, and specific.
Prefer facts over preferences.
Name the affected condition or user path.
Keep each finding to one idea.
Do not shame the author.
Do not say something is "unsafe" without naming the concrete failure.
Do not ask for perfect code when the current change improves the system and the remaining issue is optional.
Examples
Good Finding
P1 apps/web/src/app/api/private/route.ts:42 - Derive the user from the session
The route trusts userId from the request body when loading private records. Any authenticated user can send another user's ID and read their data. Derive the user ID from the server session and add a regression test for cross-user access.
Good Agent-Generated Finding
P1 .github/workflows/test.yml:31 - Keep tests blocking on pull requests
The workflow now runs tests only on pushes to main, so pull requests can merge without the regression suite. This is a CI weakening pattern common in generated fixes. Restore the pull_request trigger or add an explicit justification for changing the merge gate.
Not a Finding
The new helper name could be shorter, but it is clear, local style does not require a shorter name, and it does not create maintenance risk. Leave it out or mark it as a nit.