pr-review-expert

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 skill performs blast radius analysis, security scanning, breaking change detection, and test coverage delta calculation. Produces a reviewer-ready report with a 30+ item checklist and prioritized findings.

Core Capabilities

When to Use

Fetching the Diff

GitHub (gh CLI)

# View diff in terminal
gh pr diff 
Get PR metadata (title, body, labels, linked issues)
gh pr view  --json title,body,labels,assignees,milestone
List files changed
gh pr diff  --name-only
Check CI status
gh pr checks 
Download diff to file for analysis
gh pr diff  > /tmp/pr-.diff

GitLab (glab CLI)

# View MR diff
glab mr diff 
MR details as JSON
glab mr view  --output json
List changed files
glab mr diff  --name-only
Download diff
glab mr diff  > /tmp/mr-.diff

Workflow

Step 1 — Fetch Context

PR=123
gh pr view $PR --json title,body,labels,milestone,assignees | jq .
gh pr diff $PR --name-only
gh pr diff $PR > /tmp/pr-$PR.diff

Step 2 — Blast Radius Analysis

For each changed file, identify:

1. Direct dependents — who imports this file?

# Find all files importing a changed module
grep -r "from ['\"].*changed-module['\"]" src/ --include="*.ts" -l
grep -r "require(['\"].*changed-module" src/ --include="*.js" -l
Python
grep -r "from changed_module import\|import changed_module" . --include="*.py" -l

2. Service boundaries — does this change cross a service?

# 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

gh pr diff $PR --name-only | grep -E "types/|interfaces/|schemas/|models/"

Blast radius severity:

Step 3 — Security Scan

DIFF=/tmp/pr-$PR.diff
SQL Injection — raw query string interpolation
grep -n "query\|execute\|raw(" $DIFF | grep -E '\$\{|f"|%s|format\('
Hardcoded secrets
grep -nE "(password|secret|api_key|token|private_key)\s*=\s*['\"][^'\"]{8,}" $DIFF
AWS key pattern
grep -nE "AKIA[0-9A-Z]{16}" $DIFF
JWT secret in code
grep -nE "jwt\.sign\(.*['\"][^'\"]{20,}['\"]" $DIFF
XSS vectors
grep -n "dangerouslySetInnerHTML\|innerHTML\s*=" $DIFF
Auth bypass patterns
grep -n "bypass\|skip.*auth\|noauth\|TODO.*auth" $DIFF
Insecure hash algorithms
grep -nE "md5\(|sha1\(|createHash\(['\"]md5|createHash\(['\"]sha1" $DIFF
eval / exec
grep -nE "\beval\(|\bexec\(|\bsubprocess\.call\(" $DIFF
Prototype pollution
grep -n "__proto__\|constructor\[" $DIFF
Path traversal risk
grep -nE "path\.join\(.*req\.|readFile\(.*req\." $DIFF

Step 4 — Test Coverage Delta

# Count source vs test files changed
CHANGED_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 lines
LOGIC_LINES=$(grep "^+" /tmp/pr-$PR.diff | grep -v "^+++" | wc -l)
echo "New lines added: $LOGIC_LINES"
Run coverage locally
npm test -- --coverage --changedSince=main 2>/dev/null | tail -20
pytest --cov --cov-report=term-missing 2>/dev/null | tail -20

Coverage delta rules:

Step 5 — Breaking Change Detection

#### API Contract Changes

# OpenAPI/Swagger spec changes
grep -n "openapi\|swagger" /tmp/pr-$PR.diff | head -20
REST route removals or renames
grep "^-" /tmp/pr-$PR.diff | grep -E "router\.(get|post|put|delete|patch)\("
GraphQL schema removals
grep "^-" /tmp/pr-$PR.diff | grep -E "^-\s*(type |field |Query |Mutation )"
TypeScript interface removals
grep "^-" /tmp/pr-$PR.diff | grep -E "^-\s*(export\s+)?(interface|type) "

#### DB Schema Changes

# Migration files added
gh pr diff $PR --name-only | grep -E "migrations?/|alembic/|knex/"
Destructive operations
grep -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

# 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

# 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 dependencies
grep "^+" /tmp/pr-$PR.diff | grep -E '"[a-z@].*":\s*"[0-9^~]' | head -20
Unbounded loops
grep -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 allocations
grep -n "new Array([0-9]\{4,\}\|Buffer\.alloc" /tmp/pr-$PR.diff | grep "^+"

Ticket Linking Verification

# Extract ticket references from PR body
gh 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)
TICKET="PROJ-123"
curl -s -u "user@company.com:$JIRA_API_TOKEN" \
  "https://your-org.atlassian.net/rest/api/3/issue/$TICKET" | \
  jq '{key, summary: .fields.summary, status: .fields.status.name}'
Linear ticket
LINEAR_ID="abc-123"
curl -s -H "Authorization: $LINEAR_API_KEY" \
  -H "Content-Type: application/json" \
  --data "{\"query\": \"{ issue(id: \\\"$LINEAR_ID\\\") { title state { name } } }\"}" \
  https://api.linear.app/graphql | jq .

Complete Review Checklist (30+ Items)

## 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 services
Security: 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

Best Practices

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

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