pr-review-expert

---
name: pr-review-expert
description: Use when the user asks to review pull requests, analyze code changes, check for security issues in PRs, or assess code quality of diffs.
category: Engineering
version: 1.0.0
tools: []
---

# PR Review Expert

Structured, systematic code review for GitHub PRs and GitLab MRs — beyond style nits. Covers blast-radius analysis, security scanning, breaking-change detection, and test-coverage delta. Produces a reviewer-ready report against a 30+ item checklist with prioritized findings.

## Core Capabilities

Blast-radius analysis (which files, services, and downstream consumers could break); security scan (SQL injection, XSS, auth bypass, secret exposure, dependency vulnerabilities); test-coverage delta (new code vs. new tests ratio); breaking-change detection (API contracts, DB schema migrations, config keys); ticket-linking verification (Jira/Linear ticket exists and matches scope); performance-impact review (N+1 queries, bundle-size regression, memory allocations).

## When to Use

Before merging any PR/MR touching shared libraries, APIs, or DB schema; large PRs (>200 lines changed) needing structured review; onboarding new contributors whose PRs need thorough feedback; security-sensitive paths (auth, payments, PII); reviewing similar PRs proactively after an incident.

## Method

Read the PR's title, description, labels, linked ticket, and full diff before judging anything. Then work each of the six capability areas below in order, treating them as an evaluation checklist rather than a linear script.

**Blast radius:** for each changed file, identify direct dependents (what imports or requires this module elsewhere in the codebase), whether the change crosses a service boundary in a monorepo, and whether it touches a shared contract (types/interfaces/schemas/models directory). Classify severity: CRITICAL (shared library, DB model, auth middleware, API contract), HIGH (service used by >3 others, shared config/env vars), MEDIUM (single-service internal change, utility function), LOW (UI component, test file, docs).

**Security scan:** read the diff for the classic red flags — string-interpolated SQL/query building, hardcoded credentials or API-key-shaped strings (8+ char secrets assigned to password/secret/token-named variables), AWS access-key patterns (`AKIA...`), JWT secrets embedded in code, `dangerouslySetInnerHTML`/raw `innerHTML` assignment (XSS), auth-bypass language ("bypass", "skip auth", "TODO auth"), weak hash algorithms (MD5/SHA1) used for security purposes, `eval`/`exec`/unsandboxed subprocess calls, prototype-pollution patterns (`__proto__`, `constructor[`), and path-traversal risk (unsanitized user input reaching file-path construction or reads).

**Test coverage delta:** compare source files changed against test files changed (`.test.`/`.spec.`/`__tests__` patterns); count new lines added as a proxy for new logic surface; where a coverage tool is available, read its report. Rules: a new function without tests is a flag; deleted tests without deleted code is a flag; a coverage drop >5% blocks merge; auth/payments paths require close-to-100% coverage.

**Breaking-change detection:** for API contracts — look for OpenAPI/Swagger spec diffs, removed or renamed REST routes, removed GraphQL types/fields/Query/Mutation entries, removed TypeScript interfaces/exported types. For DB schema — new migration files, destructive operations (`DROP TABLE`, `DROP COLUMN`, `ALTER … NOT NULL` without a default, `TRUNCATE`), and removed indexes (performance-regression risk). For config — new env vars referenced in code that may be missing in production, and removed env vars that could break running instances.

**Performance impact:** DB calls (`.find`/`.findOne`/`.query`/`db.`) sitting inside a `forEach`/`map`/`for` loop (N+1 pattern); heavy new dependencies added to the manifest without justification; unbounded `while(true)` loops; sequential `await` chains that should be parallelized; large fixed-size in-memory allocations (big arrays, `Buffer.alloc`).

## Ticket-Linking Verification

Extract ticket references from the PR body (patterns like `PROJ-123` or a Linear URL). To verify a ticket exists, query the tracker's API — but keep the credential out of the command line and shell history: pass the auth token through a config read from stdin rather than a `-H`/`--data` flag containing the secret directly, so it never appears in a process list. For repeated Jira use, a `~/.netrc` entry (chmod 600) with the tracker's host, login, and token is cleaner still — no secret material in the command at all. Confirm the ticket's summary and status match the PR's stated scope.

## Complete Review Checklist (30+ Items)

**Scope & context:** PR title accurately describes the change; description explains WHY not just WHAT; linked ticket exists and matches scope; no unrelated changes (scope creep); breaking changes documented in the PR body.

**Blast radius:** all files importing changed modules identified; cross-service dependencies checked; shared types/interfaces/schemas reviewed for breakage; new env vars documented in `.env.example`; DB migrations are reversible (have a down/rollback).

**Security:** no hardcoded secrets/API keys; SQL queries parameterized, not string-interpolated; user inputs validated/sanitized before use; auth/authorization checks on all new endpoints; no XSS vectors; new dependencies checked for known CVEs; no sensitive data (PII, tokens, passwords) in logs; file uploads validated (type, size, content-type); CORS configured correctly for new endpoints.

**Testing:** new public functions have unit tests; edge cases covered (empty, null, max values); error paths tested, not just happy path; integration tests for API endpoint changes; no tests deleted without clear reason; test names clearly describe what they verify.

**Breaking changes:** no API endpoints removed without a deprecation notice; no required fields added to existing API responses; no DB columns removed without a two-phase migration plan; no env vars removed that may be set in production; backward-compatible for external API consumers.

**Performance:** no N+1 query patterns introduced; DB indexes added for new query patterns; no unbounded loops on potentially large datasets; no heavy new dependencies without justification; async operations correctly awaited; caching considered for expensive repeated operations.

**Code quality:** no dead code or unused imports; error handling present (no bare empty catch blocks); consistent with existing patterns and conventions; complex logic has explanatory comments; no unresolved TODOs (or they're tracked in a ticket).

## Output Format

Structure the review as: a one-line summary (Blast Radius / Security / Tests / Breaking Changes verdicts), then sections for **MUST FIX (Blocking)**, **SHOULD FIX (Non-blocking)**, **SUGGESTIONS**, and **LOOKS GOOD** — each finding citing the file, line, and a concrete fix. Example blocking finding: "SQL Injection risk in src/db/users.ts:42 — raw string interpolation in WHERE clause. Fix: parameterize with `db.query('SELECT * WHERE id = $1', [userId])`."

## Common Pitfalls

Reviewing style over substance (let the linter handle style — focus on logic, security, correctness); missing blast radius (a 5-line change in a shared utility can break 20 services); approving untested happy paths (always verify error paths have coverage); ignoring migration risk (NOT NULL additions need a default or a two-phase migration); indirect secret exposure (secrets leak via error messages/logs too, not just hardcoded values); skipping large PRs (if a PR is too large to review properly, request it be split).

## Best Practices

Read the linked ticket before looking at code — context prevents false positives. Check CI status before reviewing — don't review code that fails to build. Prioritize blast radius and security over style. Reproduce locally for non-trivial auth or performance changes. Label each comment clearly: "nit:", "must:", "question:", "suggestion:". Batch all comments in one review round rather than trickling feedback. Acknowledge good patterns, not just problems — specific praise improves review culture.