PR Review

PR Review

This skill covers focused, angle-specific pull request review: picking the right lens (security, performance, tests, or architecture), producing specific and severity-ranked findings, and closing with a clear merge verdict.

Workflow for Reviewing a PR

1. Pick the angle — Determine emphasis from the user's request ("security", "perf", "tests", "arch"). If unspecified, ask which angle to use, or default to security as the highest-risk default.
2. Get full file context — A diff alone routinely misses bugs that live just outside the changed lines. If only a diff is available, request the surrounding file(s) before making confident claims.
3. Review against the angle's checklist — Work through the relevant checklist below in priority order; do not mix angles unless the user asked for a full review.
4. Write specific, cited findings — Every finding names a file and line number and states the concrete defect, not a vague impression.
5. Rank by severity — Group findings as blocker, important, or nit.
6. State uncertainty explicitly — If the diff doesn't give enough context to be sure, say so and ask for more code rather than guessing.
7. End with a verdict on its own line — Safe to merge | needs changes | reject (or the architecture-specific verdict below).

Output Discipline

Applies to every review angle:

• Cite file path and line number for each finding.
• Rank findings by severity: blocker, important, nit.
• Be specific. "This looks risky" is not a finding; "src/auth.ts:42 — JWT secret read from request body, see line 41" is a finding.
• If the diff doesn't give enough context to be sure, say so explicitly and ask for the surrounding file.
• End with a verdict on its own line: Safe to merge | needs changes | reject.

Angle 1: Security

Review the PR for security defects, in order of priority:

1. Auth/authz — new endpoints or branches missing auth checks, role assumptions, IDOR (insecure direct object reference).
2. Input validation — untrusted input flowing into queries, shell commands, file paths, deserialization, or eval.
3. Injection — SQL, NoSQL, command, prompt injection, template injection.
4. Secrets — hardcoded keys/tokens, secrets in logs, secrets in client-bundled code, .env committed to the repo.
5. Output encoding — XSS via unescaped templating, raw HTML in user content, JSONP-style leaks.
6. Crypto/randomness — Math.random() used for tokens, MD5/SHA1 for password or integrity purposes, missing IVs, custom-rolled crypto.
7. Data exposure — PII in logs, overshared API responses, missing redaction.

Skip nice-to-haves; stick to defects.

Angle 2: Performance

Review for performance regressions:

1. N+1 patterns — loops doing a DB or network call per item instead of batching.
2. Hot-path allocations — new objects/arrays/maps created inside loops, regexes recompiled per call.
3. Unbounded work — missing pagination, unconstrained result sets, recursion without a depth cap.
4. Bad async — sequential awaits where Promise.all is correct, missing concurrency limits.
5. Cache misuse — cache keys that omit a relevant variable, absent or pathological TTLs.
6. Algorithm complexity — hidden O(n²) (e.g. .some() inside .map()), sorting inside a loop.

Quote the specific line, name the complexity class or bad pattern, and suggest the fix.

Angle 3: Tests

Review the test coverage on the PR:

1. Tests for new code paths — every new branch should have at least one test.
2. Edge cases — empty input, null/undefined, boundary values, errors thrown by dependencies.
3. Assertion strength — assertions that would pass with the wrong value, snapshot-only tests, tests that only check the happy path.
4. Mocking discipline — mocks that don't fail when the real interface changes, over-mocking that hides real behavior.
5. Determinism — date/time/random/network not stubbed, leading to flaky tests.
6. Test names — names that don't describe the behavior under test.

A test that exists is not the same as a test that catches regressions — read the assertions, not the test name.