File-by-file pull request review for bugs, security and quality, with a merge verdict.
---name: "pr-review-expert"description: "File-by-file pull request review for bugs, security and quality, with a merge verdict. Load whenever the user asks to review, look over or check a PR or diff before merging."--- # PR Review Expert **Tier:** POWERFUL**Category:** Engineering**Domain:** Code Review / Quality Assurance --- ## Overview Structured, systematic code review for GitHub PRs and GitLab MRs. Goes beyond style nits — this skillperforms blast radius analysis, security scanning, breaking change detection, and test coverage deltacalculation. Produces a reviewer-ready report with a 30+ item checklist and prioritized findings. --- ## Core Capabilities - **Blast radius analysis** — trace which files, services, and downstream consumers could break- **Security scan** — SQL injection, XSS, auth bypass, secret exposure, dependency vulns- **Test coverage delta** — new code vs new tests ratio- **Breaking change detection** — API contracts, DB schema migrations, config keys- **Ticket linking** — verify Jira/Linear ticket exists and matches scope- **Performance impact** — N+1 queries, bundle size regression, memory allocations --- ## When to Use - Before merging any PR/MR that touches shared libraries, APIs, or DB schema- When a PR is large (>200 lines changed) and needs structured review- Onboarding new contributors whose PRs need thorough feedback- Security-sensitive code paths (auth, payments, PII handling)- After an incident — review similar PRs proactively --- ## Fetching the Diff ### GitHub (gh CLI)```bash# View diff in terminalgh pr diff <PR_NUMBER> # Get PR metadata (title, body, labels, linked issues)gh pr view <PR_NUMBER> --json title,body,labels,assignees,milestone # List files changedgh pr diff <PR_NUMBER> --name-only # Check CI statusgh pr checks <PR_NUMBER> # Download diff to file for analysisgh pr diff <PR_NUMBER> > /tmp/pr-<PR_NUMBER>.diff``` ### GitLab (glab CLI)```bash# View MR diffglab mr diff <MR_IID> # MR details as JSONglab mr view <MR_IID> --output json # List changed filesglab mr diff <MR_IID> --name-only # Download diffglab mr diff <MR_IID> > /tmp/mr-<MR_IID>.diff``` --- ## Workflow ### Step 1 — Fetch Context ```bashPR=123gh pr view $PR --json title,body,labels,milestone,assignees | jq .gh pr diff $PR --name-onlygh pr diff $PR > /tmp/pr-$PR.diff``` ### Step 2 — Blast Radius Analysis For each changed file, identify: 1. **Direct dependents** — who imports this file?```bash# Find all files importing a changed modulegrep -r "from ['\"].*changed-module['\"]" src/ --include="*.ts" -lgrep -r "require(['\"].*changed-module" src/ --include="*.js" -l # Pythongrep -r "from changed_module import\|import changed_module" . --include="*.py" -l``` 2. **Service boundaries** — does this change cross a service?```bash# Check if changed files span multiple services (monorepo)gh pr diff $PR --name-only | cut -d/ -f1-2 | sort -u``` 3. **Shared contracts** — types, interfaces, schemas```bashgh pr diff $PR --name-only | grep -E "types/|interfaces/|schemas/|models/"``` **Blast radius 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 ### Step 3 — Security Scan ```bashDIFF=/tmp/pr-$PR.diff # SQL Injection — raw query string interpolationgrep -n "query\|execute\|raw(" $DIFF | grep -E '\$\{|f"|%s|format\(' # Hardcoded secretsgrep -nE "(password|secret|api_key|token|private_key)\s*=\s*['\"][^'\"]{8,}" $DIFF # AWS key patterngrep -nE "AKIA[0-9A-Z]{16}" $DIFF # JWT secret in codegrep -nE "jwt\.sign\(.*['\"][^'\"]{20,}['\"]" $DIFF # XSS vectorsgrep -n "dangerouslySetInnerHTML\|innerHTML\s*=" $DIFF # Auth bypass patternsgrep -n "bypass\|skip.*auth\|noauth\|TODO.*auth" $DIFF # Insecure hash algorithmsgrep -nE "md5\(|sha1\(|createHash\(['\"]md5|createHash\(['\"]sha1" $DIFF # eval / execgrep -nE "\beval\(|\bexec\(|\bsubprocess\.call\(" $DIFF # Prototype pollutiongrep -n "__proto__\|constructor\[" $DIFF # Path traversal riskgrep -nE "path\.join\(.*req\.|readFile\(.*req\." $DIFF``` ### Step 4 — Test Coverage Delta ```bash# Count source vs test files changedCHANGED_SRC=$(gh pr diff $PR --name-only | grep -vE "\.test\.|\.spec\.|__tests__")CHANGED_TESTS=$(gh pr diff $PR --name-only | grep -E "\.test\.|\.spec\.|__tests__") echo "Source files changed: $(echo "$CHANGED_SRC" | wc -w)"echo "Test files changed: $(echo "$CHANGED_TESTS" | wc -w)" # Lines of new logic vs new test linesLOGIC_LINES=$(grep "^+" /tmp/pr-$PR.diff | grep -v "^+++" | wc -l)echo "New lines added: $LOGIC_LINES" # Run coverage locallynpm test -- --coverage --changedSince=main 2>/dev/null | tail -20pytest --cov --cov-report=term-missing 2>/dev/null | tail -20``` **Coverage delta rules:**- New function without tests → flag- Deleted tests without deleted code → flag- Coverage drop >5% → block merge- Auth/payments paths → require 100% coverage ### Step 5 — Breaking Change Detection #### API Contract Changes```bash# OpenAPI/Swagger spec changesgrep -n "openapi\|swagger" /tmp/pr-$PR.diff | head -20 # REST route removals or renamesgrep "^-" /tmp/pr-$PR.diff | grep -E "router\.(get|post|put|delete|patch)\(" # GraphQL schema removalsgrep "^-" /tmp/pr-$PR.diff | grep -E "^-\s*(type |field |Query |Mutation )" # TypeScript interface removalsgrep "^-" /tmp/pr-$PR.diff | grep -E "^-\s*(export\s+)?(interface|type) "``` #### DB Schema Changes```bash# Migration files addedgh pr diff $PR --name-only | grep -E "migrations?/|alembic/|knex/" # Destructive operationsgrep -E "DROP TABLE|DROP COLUMN|ALTER.*NOT NULL|TRUNCATE" /tmp/pr-$PR.diff # Index removals (perf regression risk)grep "DROP INDEX\|remove_index" /tmp/pr-$PR.diff``` #### Config / Env Var Changes```bash# New env vars referenced in code (might be missing in prod)grep "^+" /tmp/pr-$PR.diff | grep -oE "process\.env\.[A-Z_]+" | sort -u # Removed env vars (could break running instances)grep "^-" /tmp/pr-$PR.diff | grep -oE "process\.env\.[A-Z_]+" | sort -u``` ### Step 6 — Performance Impact ```bash# N+1 query patterns (DB calls inside loops)grep -n "\.find\|\.findOne\|\.query\|db\." /tmp/pr-$PR.diff | grep "^+" | head -20# Then check surrounding context for forEach/map/for loops # Heavy new dependenciesgrep "^+" /tmp/pr-$PR.diff | grep -E '"[a-z@].*":\s*"[0-9^~]' | head -20 # Unbounded loopsgrep -n "while (true\|while(true" /tmp/pr-$PR.diff | grep "^+" # Missing await (accidentally sequential promises)grep -n "await.*await" /tmp/pr-$PR.diff | grep "^+" | head -10 # Large in-memory allocationsgrep -n "new Array([0-9]\{4,\}\|Buffer\.alloc" /tmp/pr-$PR.diff | grep "^+"``` --- ## Ticket Linking Verification ```bash# Extract ticket references from PR bodygh pr view $PR --json body | jq -r '.body' | \ grep -oE "(PROJ-[0-9]+|[A-Z]+-[0-9]+|https://linear\.app/[^)\"]+)" | sort -u # Verify Jira ticket exists (requires JIRA_API_TOKEN to be SET in the environment).# Credentials are fed to curl via a config read from stdin (-K -) so the token# never appears in argv — `ps aux` / /proc/*/cmdline can't see it, and nothing# secret lands in shell history. Never paste the raw token on the command line.TICKET="PROJ-123": "${JIRA_API_TOKEN:?JIRA_API_TOKEN must be set}"curl -s -K - "https://your-org.atlassian.net/rest/api/3/issue/$TICKET" <<EOF | \ jq '{key, summary: .fields.summary, status: .fields.status.name}'user = "user@company.com:$JIRA_API_TOKEN"EOF # Linear ticket — same pattern: the Authorization header goes through the# stdin config, not a -H flag, to keep the key out of the process list.LINEAR_ID="abc-123": "${LINEAR_API_KEY:?LINEAR_API_KEY must be set}"curl -s -K - -H "Content-Type: application/json" \ --data "{\"query\": \"{ issue(id: \\\"$LINEAR_ID\\\") { title state { name } } }\"}" \ https://api.linear.app/graphql <<EOF | jq .header = "Authorization: $LINEAR_API_KEY"EOF``` > **Security note:** for repeated Jira use, prefer a `~/.netrc` entry> (`machine your-org.atlassian.net login user@company.com password <token>`,> `chmod 600 ~/.netrc`) and call `curl -s --netrc …` — no secret material in> the command at all. --- ## Complete Review Checklist (30+ Items) ```markdown## Code Review Checklist ### Scope & Context- [ ] PR title accurately describes the change- [ ] PR description explains WHY, not just WHAT- [ ] Linked Jira/Linear ticket exists and matches scope- [ ] No unrelated changes (scope creep)- [ ] Breaking changes documented in PR body ### Blast Radius- [ ] Identified all files importing changed modules- [ ] Cross-service dependencies checked- [ ] Shared types/interfaces/schemas reviewed for breakage- [ ] New env vars documented in .env.example- [ ] DB migrations are reversible (have down() / rollback) ### Security- [ ] No hardcoded secrets or API keys- [ ] SQL queries use parameterized inputs (no string interpolation)- [ ] User inputs validated/sanitized before use- [ ] Auth/authorization checks on all new endpoints- [ ] No XSS vectors (innerHTML, dangerouslySetInnerHTML)- [ ] New dependencies checked for known CVEs- [ ] No sensitive data in logs (PII, tokens, passwords)- [ ] 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 deprecation notice- [ ] No required fields added to existing API responses- [ ] No DB columns removed without 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 tracked in ticket)``` --- ## Output Format Structure your review comment as: ```## PR Review: [PR Title] (#NUMBER) Blast Radius: HIGH — changes lib/auth used by 5 servicesSecurity: 1 finding (medium severity)Tests: Coverage delta +2%Breaking Changes: None detected --- MUST FIX (Blocking) --- 1. SQL Injection risk in src/db/users.ts:42 Raw string interpolation in WHERE clause. Fix: db.query("SELECT * WHERE id = $1", [userId]) --- SHOULD FIX (Non-blocking) --- 2. Missing auth check on POST /api/admin/reset No role verification before destructive operation. --- SUGGESTIONS --- 3. N+1 pattern in src/services/reports.ts:88 findUser() called inside results.map() — batch with findManyUsers(ids) --- LOOKS GOOD ---- Test coverage for new auth flow is thorough- DB migration has proper down() rollback method- Error handling consistent with rest of codebase``` --- ## 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 two-phase migration- **Indirect secret exposure** — secrets in error messages/logs, not just hardcoded values- **Skipping large PRs** — if a PR is too large to review properly, request it be split --- ## Best Practices 1. Read the linked ticket before looking at code — context prevents false positives2. Check CI status before reviewing — don't review code that fails to build3. Prioritize blast radius and security over style4. Reproduce locally for non-trivial auth or performance changes5. Label each comment clearly: "nit:", "must:", "question:", "suggestion:"6. Batch all comments in one review round — don't trickle feedback7. Acknowledge good patterns, not just problems — specific praise improves culture