mindrally/skills/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.

Angle 4: Architecture

Pull back from line-level concerns to review the shape of the change:

1. Boundary drift — where did the seam between layers move? Did UI start reaching into the DB? Did domain types start importing transport types?
2. Premature abstraction — interfaces, factories, or config layers with only one implementation; flag these as debt.
3. Coupling — utilities now importing from feature modules, shared mutable state being introduced.
4. Scalability — if this code path goes 10x, what breaks first?
5. Reversibility — if this turns out wrong in a month, how hard is the rollback? Call out one-way doors explicitly.
6. Naming — types/functions named for the implementation (UserManagerImplV2) rather than the role (UserDirectory).

End with: Architecturally sound | needs trim | re-think before merging.

Example Finding Format

src/api/orders.ts:118 (blocker, security)
  Order lookup uses `req.params.orderId` directly in a WHERE clause
  built via string concatenation — SQL injection. Use a parameterized
  query or the existing `db.orders.findById()` helper instead.
 
src/api/orders.ts:142 (nit, tests)
  `createOrder` has no test for the case where `items` is an empty
  array; current tests only cover the happy path with 1-3 items.
 
Verdict: needs changes

Reviewing Multiple Angles at Once

• When the user asks for a "full review" rather than one specific angle, run all four checklists in sequence rather than blending them into one undifferentiated pass — each angle catches different classes of defect, and keeping them separate keeps findings traceable to a specific concern.
• Present each angle under its own heading with its own verdict line; do not merge four verdicts into one summary sentence.
• If time or context is limited, prioritize security first, then tests, then performance, then architecture — a security defect that ships is categorically worse than an architectural wart.

Common Pitfalls in Review

• Approving based on the diff's intent ("looks like it adds validation") rather than reading what the code actually does.
• Treating a passing test suite as proof of correctness — a test suite only proves the tests that exist pass, not that the right tests exist.
• Letting scope creep into the review: flagging pre-existing issues untouched by the diff is fine to mention separately, but shouldn't block the PR under review.
• Giving a verdict without having seen enough of the surrounding file to justify it — ask for more context instead of guessing.