Review a code diff or pull request for minimal blast radius, scope discipline, security-sensitive domains (auth, billing, admin, secrets), and test coverage on new logic. Use when the user asks to "review this PR/diff", "does this change look safe to merge", or pastes a patch for feedback before merging. Do NOT use for reviewing prose, product specs, or non-code documents — use editorial-review for those.
v1 · authored by Gryz · engineering
Use it from any MCP-connected agent
Connect Gryz MCP, then call list_skills and get_skill({ name: "code-review" }). Everyone can use Library skills; Pro can fork and edit this one, and Pro Plus can create your own from scratch.
# Code review
Review a code diff or pull request the way a careful senior engineer would:
scope discipline first, security-sensitive domains second, tests third.
## When to use
- "Review this PR/diff", "does this change look safe to merge", or the user
pastes a patch or a link to a diff for feedback before merging.
## Do NOT use for
- Reviewing prose, product specs, or design docs — use editorial-review.
- Writing the code yourself unprompted — review what's there; don't rewrite
it unless asked.
## Review checklist, in priority order
1. **Blast radius.** Does the diff change only what the stated task needs?
Flag any unrelated rename, reformat, refactor, or "while I'm here" cleanup
— even a good one — as out of scope for this change.
2. **Security-sensitive domains.** Extra scrutiny on anything touching auth,
session/token handling, billing/webhooks, admin routes, secrets/env vars,
or a database migration:
- No hardcoded credentials, ever.
- No secret exposed to client-side code.
- Auth/ownership checks present on every new read or write path — not
assumed from a caller-supplied id.
- Webhook handlers verify signatures and are idempotent.
- A migration only adds a new file — it never edits one already applied.
3. **Correctness.** Does the change actually do what the PR description
says? Look for off-by-one errors, unhandled error branches, and
inconsistent state left behind if a step fails partway through.
4. **Test coverage.** Does new critical logic (pure functions, tier/limit
math, validation, anything auth/billing touches) have a test? Flag its
absence as a real finding, not a nice-to-have.
5. **Conventions.** Does the diff match the surrounding file's existing
style (naming, indentation, import order) rather than imposing a new one?
## How to respond
Group findings by severity — critical (blocks merge: security, correctness),
then scope/blast-radius, then minor/style — and cite the specific line or
hunk for each. Don't restate the whole diff back; point at what's wrong and
why.
## Examples
**Finding:** "Critical — `app/api/admin/users/route.ts` line 42: this route
reads `req.headers.get('x-user-id')` to decide the target user instead of the
authenticated session. Any caller can act as any other user by setting that
header."
**Finding:** "Scope — this diff also reformats `lib/utils.ts` (200 lines
changed) unrelated to the stated bug fix. Recommend splitting into a separate
PR so the real fix is reviewable."