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

- **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 terminal
gh 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 changed
gh pr diff <PR_NUMBER> --name-only

# Check CI status
gh pr checks <PR_NUMBER>

# Download diff to file for analysis
gh pr diff <PR_NUMBER> > /tmp/pr-<PR_NUMBER>.diff
```

### GitLab (glab CLI)
```bash
# View MR diff
glab mr diff <MR_IID>

# MR details as JSON
glab mr view <MR_IID> --output json

# List changed files
glab mr diff <MR_IID> --name-only

# Download diff
glab mr diff <MR_IID> > /tmp/mr-<MR_IID>.diff
```

---

## Workflow

### Step 1 — Fetch Context

```bash
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?
```bash
# 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?
```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
```bash
gh 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

```bash
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

```bash
# 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:**
- 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 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
```bash
# 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
```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 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

```bash
# 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 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 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

- **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 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

{
  "format": "aiskill/v1",
  "skill": {
    "name": "pr-review-expert",
    "slug": "pr-review-expert",
    "version": "1.0.0",
    "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": "Development",
    "tags": [
      "dev-workflow",
      "security"
    ],
    "platforms": [
      "chatgpt",
      "claude-code",
      "cursor"
    ],
    "tested_environment": "Agent Skills convention (SKILL.md) — works with Claude Code, Cursor, ChatGPT/Codex and compatible agents",
    "license": "MIT"
  },
  "prompts": {
    "system": "# PR Review Expert\r\n\r\n**Tier:** POWERFUL\r\n**Category:** Engineering\r\n**Domain:** Code Review / Quality Assurance\r\n\r\n---\r\n\r\n## Overview\r\n\r\nStructured, systematic code review for GitHub PRs and GitLab MRs. Goes beyond style nits — this skill\r\nperforms blast radius analysis, security scanning, breaking change detection, and test coverage delta\r\ncalculation. Produces a reviewer-ready report with a 30+ item checklist and prioritized findings.\r\n\r\n---\r\n\r\n## Core Capabilities\r\n\r\n- **Blast radius analysis** — trace which files, services, and downstream consumers could break\r\n- **Security scan** — SQL injection, XSS, auth bypass, secret exposure, dependency vulns\r\n- **Test coverage delta** — new code vs new tests ratio\r\n- **Breaking change detection** — API contracts, DB schema migrations, config keys\r\n- **Ticket linking** — verify Jira/Linear ticket exists and matches scope\r\n- **Performance impact** — N+1 queries, bundle size regression, memory allocations\r\n\r\n---\r\n\r\n## When to Use\r\n\r\n- Before merging any PR/MR that touches shared libraries, APIs, or DB schema\r\n- When a PR is large (>200 lines changed) and needs structured review\r\n- Onboarding new contributors whose PRs need thorough feedback\r\n- Security-sensitive code paths (auth, payments, PII handling)\r\n- After an incident — review similar PRs proactively\r\n\r\n---\r\n\r\n## Fetching the Diff\r\n\r\n### GitHub (gh CLI)\r\n```bash\r\n# View diff in terminal\r\ngh pr diff <PR_NUMBER>\r\n\r\n# Get PR metadata (title, body, labels, linked issues)\r\ngh pr view <PR_NUMBER> --json title,body,labels,assignees,milestone\r\n\r\n# List files changed\r\ngh pr diff <PR_NUMBER> --name-only\r\n\r\n# Check CI status\r\ngh pr checks <PR_NUMBER>\r\n\r\n# Download diff to file for analysis\r\ngh pr diff <PR_NUMBER> > /tmp/pr-<PR_NUMBER>.diff\r\n```\r\n\r\n### GitLab (glab CLI)\r\n```bash\r\n# View MR diff\r\nglab mr diff <MR_IID>\r\n\r\n# MR details as JSON\r\nglab mr view <MR_IID> --output json\r\n\r\n# List changed files\r\nglab mr diff <MR_IID> --name-only\r\n\r\n# Download diff\r\nglab mr diff <MR_IID> > /tmp/mr-<MR_IID>.diff\r\n```\r\n\r\n---\r\n\r\n## Workflow\r\n\r\n### Step 1 — Fetch Context\r\n\r\n```bash\r\nPR=123\r\ngh pr view $PR --json title,body,labels,milestone,assignees | jq .\r\ngh pr diff $PR --name-only\r\ngh pr diff $PR > /tmp/pr-$PR.diff\r\n```\r\n\r\n### Step 2 — Blast Radius Analysis\r\n\r\nFor each changed file, identify:\r\n\r\n1. **Direct dependents** — who imports this file?\r\n```bash\r\n# Find all files importing a changed module\r\ngrep -r \"from ['\\\"].*changed-module['\\\"]\" src/ --include=\"*.ts\" -l\r\ngrep -r \"require(['\\\"].*changed-module\" src/ --include=\"*.js\" -l\r\n\r\n# Python\r\ngrep -r \"from changed_module import\\|import changed_module\" . --include=\"*.py\" -l\r\n```\r\n\r\n2. **Service boundaries** — does this change cross a service?\r\n```bash\r\n# Check if changed files span multiple services (monorepo)\r\ngh pr diff $PR --name-only | cut -d/ -f1-2 | sort -u\r\n```\r\n\r\n3. **Shared contracts** — types, interfaces, schemas\r\n```bash\r\ngh pr diff $PR --name-only | grep -E \"types/|interfaces/|schemas/|models/\"\r\n```\r\n\r\n**Blast radius severity:**\r\n- CRITICAL — shared library, DB model, auth middleware, API contract\r\n- HIGH     — service used by >3 others, shared config, env vars\r\n- MEDIUM   — single service internal change, utility function\r\n- LOW      — UI component, test file, docs\r\n\r\n### Step 3 — Security Scan\r\n\r\n```bash\r\nDIFF=/tmp/pr-$PR.diff\r\n\r\n# SQL Injection — raw query string interpolation\r\ngrep -n \"query\\|execute\\|raw(\" $DIFF | grep -E '\\$\\{|f\"|%s|format\\('\r\n\r\n# Hardcoded secrets\r\ngrep -nE \"(password|secret|api_key|token|private_key)\\s*=\\s*['\\\"][^'\\\"]{8,}\" $DIFF\r\n\r\n# AWS key pattern\r\ngrep -nE \"AKIA[0-9A-Z]{16}\" $DIFF\r\n\r\n# JWT secret in code\r\ngrep -nE \"jwt\\.sign\\(.*['\\\"][^'\\\"]{20,}['\\\"]\" $DIFF\r\n\r\n# XSS vectors\r\ngrep -n \"dangerouslySetInnerHTML\\|innerHTML\\s*=\" $DIFF\r\n\r\n# Auth bypass patterns\r\ngrep -n \"bypass\\|skip.*auth\\|noauth\\|TODO.*auth\" $DIFF\r\n\r\n# Insecure hash algorithms\r\ngrep -nE \"md5\\(|sha1\\(|createHash\\(['\\\"]md5|createHash\\(['\\\"]sha1\" $DIFF\r\n\r\n# eval / exec\r\ngrep -nE \"\\beval\\(|\\bexec\\(|\\bsubprocess\\.call\\(\" $DIFF\r\n\r\n# Prototype pollution\r\ngrep -n \"__proto__\\|constructor\\[\" $DIFF\r\n\r\n# Path traversal risk\r\ngrep -nE \"path\\.join\\(.*req\\.|readFile\\(.*req\\.\" $DIFF\r\n```\r\n\r\n### Step 4 — Test Coverage Delta\r\n\r\n```bash\r\n# Count source vs test files changed\r\nCHANGED_SRC=$(gh pr diff $PR --name-only | grep -vE \"\\.test\\.|\\.spec\\.|__tests__\")\r\nCHANGED_TESTS=$(gh pr diff $PR --name-only | grep -E \"\\.test\\.|\\.spec\\.|__tests__\")\r\n\r\necho \"Source files changed: $(echo \"$CHANGED_SRC\" | wc -w)\"\r\necho \"Test files changed:   $(echo \"$CHANGED_TESTS\" | wc -w)\"\r\n\r\n# Lines of new logic vs new test lines\r\nLOGIC_LINES=$(grep \"^+\" /tmp/pr-$PR.diff | grep -v \"^+++\" | wc -l)\r\necho \"New lines added: $LOGIC_LINES\"\r\n\r\n# Run coverage locally\r\nnpm test -- --coverage --changedSince=main 2>/dev/null | tail -20\r\npytest --cov --cov-report=term-missing 2>/dev/null | tail -20\r\n```\r\n\r\n**Coverage delta rules:**\r\n- New function without tests → flag\r\n- Deleted tests without deleted code → flag\r\n- Coverage drop >5% → block merge\r\n- Auth/payments paths → require 100% coverage\r\n\r\n### Step 5 — Breaking Change Detection\r\n\r\n#### API Contract Changes\r\n```bash\r\n# OpenAPI/Swagger spec changes\r\ngrep -n \"openapi\\|swagger\" /tmp/pr-$PR.diff | head -20\r\n\r\n# REST route removals or renames\r\ngrep \"^-\" /tmp/pr-$PR.diff | grep -E \"router\\.(get|post|put|delete|patch)\\(\"\r\n\r\n# GraphQL schema removals\r\ngrep \"^-\" /tmp/pr-$PR.diff | grep -E \"^-\\s*(type |field |Query |Mutation )\"\r\n\r\n# TypeScript interface removals\r\ngrep \"^-\" /tmp/pr-$PR.diff | grep -E \"^-\\s*(export\\s+)?(interface|type) \"\r\n```\r\n\r\n#### DB Schema Changes\r\n```bash\r\n# Migration files added\r\ngh pr diff $PR --name-only | grep -E \"migrations?/|alembic/|knex/\"\r\n\r\n# Destructive operations\r\ngrep -E \"DROP TABLE|DROP COLUMN|ALTER.*NOT NULL|TRUNCATE\" /tmp/pr-$PR.diff\r\n\r\n# Index removals (perf regression risk)\r\ngrep \"DROP INDEX\\|remove_index\" /tmp/pr-$PR.diff\r\n```\r\n\r\n#### Config / Env Var Changes\r\n```bash\r\n# New env vars referenced in code (might be missing in prod)\r\ngrep \"^+\" /tmp/pr-$PR.diff | grep -oE \"process\\.env\\.[A-Z_]+\" | sort -u\r\n\r\n# Removed env vars (could break running instances)\r\ngrep \"^-\" /tmp/pr-$PR.diff | grep -oE \"process\\.env\\.[A-Z_]+\" | sort -u\r\n```\r\n\r\n### Step 6 — Performance Impact\r\n\r\n```bash\r\n# N+1 query patterns (DB calls inside loops)\r\ngrep -n \"\\.find\\|\\.findOne\\|\\.query\\|db\\.\" /tmp/pr-$PR.diff | grep \"^+\" | head -20\r\n# Then check surrounding context for forEach/map/for loops\r\n\r\n# Heavy new dependencies\r\ngrep \"^+\" /tmp/pr-$PR.diff | grep -E '\"[a-z@].*\":\\s*\"[0-9^~]' | head -20\r\n\r\n# Unbounded loops\r\ngrep -n \"while (true\\|while(true\" /tmp/pr-$PR.diff | grep \"^+\"\r\n\r\n# Missing await (accidentally sequential promises)\r\ngrep -n \"await.*await\" /tmp/pr-$PR.diff | grep \"^+\" | head -10\r\n\r\n# Large in-memory allocations\r\ngrep -n \"new Array([0-9]\\{4,\\}\\|Buffer\\.alloc\" /tmp/pr-$PR.diff | grep \"^+\"\r\n```\r\n\r\n---\r\n\r\n## Ticket Linking Verification\r\n\r\n```bash\r\n# Extract ticket references from PR body\r\ngh pr view $PR --json body | jq -r '.body' | \\\r\n  grep -oE \"(PROJ-[0-9]+|[A-Z]+-[0-9]+|https://linear\\.app/[^)\\\"]+)\" | sort -u\r\n\r\n# Verify Jira ticket exists (requires JIRA_API_TOKEN to be SET in the environment).\r\n# Credentials are fed to curl via a config read from stdin (-K -) so the token\r\n# never appears in argv — `ps aux` / /proc/*/cmdline can't see it, and nothing\r\n# secret lands in shell history. Never paste the raw token on the command line.\r\nTICKET=\"PROJ-123\"\r\n: \"${JIRA_API_TOKEN:?JIRA_API_TOKEN must be set}\"\r\ncurl -s -K - \"https://your-org.atlassian.net/rest/api/3/issue/$TICKET\" <<EOF | \\\r\n  jq '{key, summary: .fields.summary, status: .fields.status.name}'\r\nuser = \"user@company.com:$JIRA_API_TOKEN\"\r\nEOF\r\n\r\n# Linear ticket — same pattern: the Authorization header goes through the\r\n# stdin config, not a -H flag, to keep the key out of the process list.\r\nLINEAR_ID=\"abc-123\"\r\n: \"${LINEAR_API_KEY:?LINEAR_API_KEY must be set}\"\r\ncurl -s -K - -H \"Content-Type: application/json\" \\\r\n  --data \"{\\\"query\\\": \\\"{ issue(id: \\\\\\\"$LINEAR_ID\\\\\\\") { title state { name } } }\\\"}\" \\\r\n  https://api.linear.app/graphql <<EOF | jq .\r\nheader = \"Authorization: $LINEAR_API_KEY\"\r\nEOF\r\n```\r\n\r\n> **Security note:** for repeated Jira use, prefer a `~/.netrc` entry\r\n> (`machine your-org.atlassian.net login user@company.com password <token>`,\r\n> `chmod 600 ~/.netrc`) and call `curl -s --netrc …` — no secret material in\r\n> the command at all.\r\n\r\n---\r\n\r\n## Complete Review Checklist (30+ Items)\r\n\r\n```markdown\r\n## Code Review Checklist\r\n\r\n### Scope & Context\r\n- [ ] PR title accurately describes the change\r\n- [ ] PR description explains WHY, not just WHAT\r\n- [ ] Linked Jira/Linear ticket exists and matches scope\r\n- [ ] No unrelated changes (scope creep)\r\n- [ ] Breaking changes documented in PR body\r\n\r\n### Blast Radius\r\n- [ ] Identified all files importing changed modules\r\n- [ ] Cross-service dependencies checked\r\n- [ ] Shared types/interfaces/schemas reviewed for breakage\r\n- [ ] New env vars documented in .env.example\r\n- [ ] DB migrations are reversible (have down() / rollback)\r\n\r\n### Security\r\n- [ ] No hardcoded secrets or API keys\r\n- [ ] SQL queries use parameterized inputs (no string interpolation)\r\n- [ ] User inputs validated/sanitized before use\r\n- [ ] Auth/authorization checks on all new endpoints\r\n- [ ] No XSS vectors (innerHTML, dangerouslySetInnerHTML)\r\n- [ ] New dependencies checked for known CVEs\r\n- [ ] No sensitive data in logs (PII, tokens, passwords)\r\n- [ ] File uploads validated (type, size, content-type)\r\n- [ ] CORS configured correctly for new endpoints\r\n\r\n### Testing\r\n- [ ] New public functions have unit tests\r\n- [ ] Edge cases covered (empty, null, max values)\r\n- [ ] Error paths tested (not just happy path)\r\n- [ ] Integration tests for API endpoint changes\r\n- [ ] No tests deleted without clear reason\r\n- [ ] Test names clearly describe what they verify\r\n\r\n### Breaking Changes\r\n- [ ] No API endpoints removed without deprecation notice\r\n- [ ] No required fields added to existing API responses\r\n- [ ] No DB columns removed without two-phase migration plan\r\n- [ ] No env vars removed that may be set in production\r\n- [ ] Backward-compatible for external API consumers\r\n\r\n### Performance\r\n- [ ] No N+1 query patterns introduced\r\n- [ ] DB indexes added for new query patterns\r\n- [ ] No unbounded loops on potentially large datasets\r\n- [ ] No heavy new dependencies without justification\r\n- [ ] Async operations correctly awaited\r\n- [ ] Caching considered for expensive repeated operations\r\n\r\n### Code Quality\r\n- [ ] No dead code or unused imports\r\n- [ ] Error handling present (no bare empty catch blocks)\r\n- [ ] Consistent with existing patterns and conventions\r\n- [ ] Complex logic has explanatory comments\r\n- [ ] No unresolved TODOs (or tracked in ticket)\r\n```\r\n\r\n---\r\n\r\n## Output Format\r\n\r\nStructure your review comment as:\r\n\r\n```\r\n## PR Review: [PR Title] (#NUMBER)\r\n\r\nBlast Radius: HIGH — changes lib/auth used by 5 services\r\nSecurity: 1 finding (medium severity)\r\nTests: Coverage delta +2%\r\nBreaking Changes: None detected\r\n\r\n--- MUST FIX (Blocking) ---\r\n\r\n1. SQL Injection risk in src/db/users.ts:42\r\n   Raw string interpolation in WHERE clause.\r\n   Fix: db.query(\"SELECT * WHERE id = $1\", [userId])\r\n\r\n--- SHOULD FIX (Non-blocking) ---\r\n\r\n2. Missing auth check on POST /api/admin/reset\r\n   No role verification before destructive operation.\r\n\r\n--- SUGGESTIONS ---\r\n\r\n3. N+1 pattern in src/services/reports.ts:88\r\n   findUser() called inside results.map() — batch with findManyUsers(ids)\r\n\r\n--- LOOKS GOOD ---\r\n- Test coverage for new auth flow is thorough\r\n- DB migration has proper down() rollback method\r\n- Error handling consistent with rest of codebase\r\n```\r\n\r\n---\r\n\r\n## Common Pitfalls\r\n\r\n- **Reviewing style over substance** — let the linter handle style; focus on logic, security, correctness\r\n- **Missing blast radius** — a 5-line change in a shared utility can break 20 services\r\n- **Approving untested happy paths** — always verify error paths have coverage\r\n- **Ignoring migration risk** — NOT NULL additions need a default or two-phase migration\r\n- **Indirect secret exposure** — secrets in error messages/logs, not just hardcoded values\r\n- **Skipping large PRs** — if a PR is too large to review properly, request it be split\r\n\r\n---\r\n\r\n## Best Practices\r\n\r\n1. Read the linked ticket before looking at code — context prevents false positives\r\n2. Check CI status before reviewing — don't review code that fails to build\r\n3. Prioritize blast radius and security over style\r\n4. Reproduce locally for non-trivial auth or performance changes\r\n5. Label each comment clearly: \"nit:\", \"must:\", \"question:\", \"suggestion:\"\r\n6. Batch all comments in one review round — don't trickle feedback\r\n7. Acknowledge good patterns, not just problems — specific praise improves culture",
    "user_template": ""
  },
  "variables": [],
  "install": {
    "steps": [
      {
        "title": "Download & unzip",
        "detail": "Download the .zip package from SkillVault and unzip it. The pr-review-expert/ folder contains SKILL.md plus any companion scripts, references and assets."
      },
      {
        "title": "Copy into your agent skills directory",
        "detail": "Move the folder into your agent's skills directory — e.g. ~/.claude/skills/ (Claude Code), .codex/skills/, .cursor/skills/ or your project's .claude/skills/ folder — then restart or reload the session."
      },
      {
        "title": "Use it",
        "detail": "Describe the task in natural language; the agent loads the skill automatically when the request matches the skill description."
      }
    ]
  },
  "usage": {
    "steps": []
  },
  "metadata": {
    "author": "SkillVault",
    "source": "https://skillvault.top/skills/pr-review-expert",
    "exported_at": "2026-11-03T07:45:00.412Z"
  }
}

format: aiskill/v1
skill:
  name: pr-review-expert
  slug: pr-review-expert
  version: 1.0.0
  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: Development
  tags:
    - dev-workflow
    - security
  platforms:
    - chatgpt
    - claude-code
    - cursor
  tested_environment: >-
    Agent Skills convention (SKILL.md) — works with Claude Code, Cursor, ChatGPT/Codex and
    compatible agents
  license: MIT
prompts:
  system: "# PR Review Expert\r\n\r\n**Tier:** POWERFUL\r\n**Category:** Engineering\r\n**Domain:** Code Review / Quality Assurance\r\n\r\n---\r\n\r\n## Overview\r\n\r\nStructured, systematic code review for GitHub PRs and GitLab MRs. Goes beyond style nits — this skill\r\nperforms blast radius analysis, security scanning, breaking change detection, and test coverage delta\r\ncalculation. Produces a reviewer-ready report with a 30+ item checklist and prioritized findings.\r\n\r\n---\r\n\r\n## Core Capabilities\r\n\r\n- **Blast radius analysis** — trace which files, services, and downstream consumers could break\r\n- **Security scan** — SQL injection, XSS, auth bypass, secret exposure, dependency vulns\r\n- **Test coverage delta** — new code vs new tests ratio\r\n- **Breaking change detection** — API contracts, DB schema migrations, config keys\r\n- **Ticket linking** — verify Jira/Linear ticket exists and matches scope\r\n- **Performance impact** — N+1 queries, bundle size regression, memory allocations\r\n\r\n---\r\n\r\n## When to Use\r\n\r\n- Before merging any PR/MR that touches shared libraries, APIs, or DB schema\r\n- When a PR is large (>200 lines changed) and needs structured review\r\n- Onboarding new contributors whose PRs need thorough feedback\r\n- Security-sensitive code paths (auth, payments, PII handling)\r\n- After an incident — review similar PRs proactively\r\n\r\n---\r\n\r\n## Fetching the Diff\r\n\r\n### GitHub (gh CLI)\r\n```bash\r\n# View diff in terminal\r\ngh pr diff <PR_NUMBER>\r\n\r\n# Get PR metadata (title, body, labels, linked issues)\r\ngh pr view <PR_NUMBER> --json title,body,labels,assignees,milestone\r\n\r\n# List files changed\r\ngh pr diff <PR_NUMBER> --name-only\r\n\r\n# Check CI status\r\ngh pr checks <PR_NUMBER>\r\n\r\n# Download diff to file for analysis\r\ngh pr diff <PR_NUMBER> > /tmp/pr-<PR_NUMBER>.diff\r\n```\r\n\r\n### GitLab (glab CLI)\r\n```bash\r\n# View MR diff\r\nglab mr diff <MR_IID>\r\n\r\n# MR details as JSON\r\nglab mr view <MR_IID> --output json\r\n\r\n# List changed files\r\nglab mr diff <MR_IID> --name-only\r\n\r\n# Download diff\r\nglab mr diff <MR_IID> > /tmp/mr-<MR_IID>.diff\r\n```\r\n\r\n---\r\n\r\n## Workflow\r\n\r\n### Step 1 — Fetch Context\r\n\r\n```bash\r\nPR=123\r\ngh pr view $PR --json title,body,labels,milestone,assignees | jq .\r\ngh pr diff $PR --name-only\r\ngh pr diff $PR > /tmp/pr-$PR.diff\r\n```\r\n\r\n### Step 2 — Blast Radius Analysis\r\n\r\nFor each changed file, identify:\r\n\r\n1. **Direct dependents** — who imports this file?\r\n```bash\r\n# Find all files importing a changed module\r\ngrep -r \"from ['\\\"].*changed-module['\\\"]\" src/ --include=\"*.ts\" -l\r\ngrep -r \"require(['\\\"].*changed-module\" src/ --include=\"*.js\" -l\r\n\r\n# Python\r\ngrep -r \"from changed_module import\\|import changed_module\" . --include=\"*.py\" -l\r\n```\r\n\r\n2. **Service boundaries** — does this change cross a service?\r\n```bash\r\n# Check if changed files span multiple services (monorepo)\r\ngh pr diff $PR --name-only | cut -d/ -f1-2 | sort -u\r\n```\r\n\r\n3. **Shared contracts** — types, interfaces, schemas\r\n```bash\r\ngh pr diff $PR --name-only | grep -E \"types/|interfaces/|schemas/|models/\"\r\n```\r\n\r\n**Blast radius severity:**\r\n- CRITICAL — shared library, DB model, auth middleware, API contract\r\n- HIGH     — service used by >3 others, shared config, env vars\r\n- MEDIUM   — single service internal change, utility function\r\n- LOW      — UI component, test file, docs\r\n\r\n### Step 3 — Security Scan\r\n\r\n```bash\r\nDIFF=/tmp/pr-$PR.diff\r\n\r\n# SQL Injection — raw query string interpolation\r\ngrep -n \"query\\|execute\\|raw(\" $DIFF | grep -E '\\$\\{|f\"|%s|format\\('\r\n\r\n# Hardcoded secrets\r\ngrep -nE \"(password|secret|api_key|token|private_key)\\s*=\\s*['\\\"][^'\\\"]{8,}\" $DIFF\r\n\r\n# AWS key pattern\r\ngrep -nE \"AKIA[0-9A-Z]{16}\" $DIFF\r\n\r\n# JWT secret in code\r\ngrep -nE \"jwt\\.sign\\(.*['\\\"][^'\\\"]{20,}['\\\"]\" $DIFF\r\n\r\n# XSS vectors\r\ngrep -n \"dangerouslySetInnerHTML\\|innerHTML\\s*=\" $DIFF\r\n\r\n# Auth bypass patterns\r\ngrep -n \"bypass\\|skip.*auth\\|noauth\\|TODO.*auth\" $DIFF\r\n\r\n# Insecure hash algorithms\r\ngrep -nE \"md5\\(|sha1\\(|createHash\\(['\\\"]md5|createHash\\(['\\\"]sha1\" $DIFF\r\n\r\n# eval / exec\r\ngrep -nE \"\\beval\\(|\\bexec\\(|\\bsubprocess\\.call\\(\" $DIFF\r\n\r\n# Prototype pollution\r\ngrep -n \"__proto__\\|constructor\\[\" $DIFF\r\n\r\n# Path traversal risk\r\ngrep -nE \"path\\.join\\(.*req\\.|readFile\\(.*req\\.\" $DIFF\r\n```\r\n\r\n### Step 4 — Test Coverage Delta\r\n\r\n```bash\r\n# Count source vs test files changed\r\nCHANGED_SRC=$(gh pr diff $PR --name-only | grep -vE \"\\.test\\.|\\.spec\\.|__tests__\")\r\nCHANGED_TESTS=$(gh pr diff $PR --name-only | grep -E \"\\.test\\.|\\.spec\\.|__tests__\")\r\n\r\necho \"Source files changed: $(echo \"$CHANGED_SRC\" | wc -w)\"\r\necho \"Test files changed:   $(echo \"$CHANGED_TESTS\" | wc -w)\"\r\n\r\n# Lines of new logic vs new test lines\r\nLOGIC_LINES=$(grep \"^+\" /tmp/pr-$PR.diff | grep -v \"^+++\" | wc -l)\r\necho \"New lines added: $LOGIC_LINES\"\r\n\r\n# Run coverage locally\r\nnpm test -- --coverage --changedSince=main 2>/dev/null | tail -20\r\npytest --cov --cov-report=term-missing 2>/dev/null | tail -20\r\n```\r\n\r\n**Coverage delta rules:**\r\n- New function without tests → flag\r\n- Deleted tests without deleted code → flag\r\n- Coverage drop >5% → block merge\r\n- Auth/payments paths → require 100% coverage\r\n\r\n### Step 5 — Breaking Change Detection\r\n\r\n#### API Contract Changes\r\n```bash\r\n# OpenAPI/Swagger spec changes\r\ngrep -n \"openapi\\|swagger\" /tmp/pr-$PR.diff | head -20\r\n\r\n# REST route removals or renames\r\ngrep \"^-\" /tmp/pr-$PR.diff | grep -E \"router\\.(get|post|put|delete|patch)\\(\"\r\n\r\n# GraphQL schema removals\r\ngrep \"^-\" /tmp/pr-$PR.diff | grep -E \"^-\\s*(type |field |Query |Mutation )\"\r\n\r\n# TypeScript interface removals\r\ngrep \"^-\" /tmp/pr-$PR.diff | grep -E \"^-\\s*(export\\s+)?(interface|type) \"\r\n```\r\n\r\n#### DB Schema Changes\r\n```bash\r\n# Migration files added\r\ngh pr diff $PR --name-only | grep -E \"migrations?/|alembic/|knex/\"\r\n\r\n# Destructive operations\r\ngrep -E \"DROP TABLE|DROP COLUMN|ALTER.*NOT NULL|TRUNCATE\" /tmp/pr-$PR.diff\r\n\r\n# Index removals (perf regression risk)\r\ngrep \"DROP INDEX\\|remove_index\" /tmp/pr-$PR.diff\r\n```\r\n\r\n#### Config / Env Var Changes\r\n```bash\r\n# New env vars referenced in code (might be missing in prod)\r\ngrep \"^+\" /tmp/pr-$PR.diff | grep -oE \"process\\.env\\.[A-Z_]+\" | sort -u\r\n\r\n# Removed env vars (could break running instances)\r\ngrep \"^-\" /tmp/pr-$PR.diff | grep -oE \"process\\.env\\.[A-Z_]+\" | sort -u\r\n```\r\n\r\n### Step 6 — Performance Impact\r\n\r\n```bash\r\n# N+1 query patterns (DB calls inside loops)\r\ngrep -n \"\\.find\\|\\.findOne\\|\\.query\\|db\\.\" /tmp/pr-$PR.diff | grep \"^+\" | head -20\r\n# Then check surrounding context for forEach/map/for loops\r\n\r\n# Heavy new dependencies\r\ngrep \"^+\" /tmp/pr-$PR.diff | grep -E '\"[a-z@].*\":\\s*\"[0-9^~]' | head -20\r\n\r\n# Unbounded loops\r\ngrep -n \"while (true\\|while(true\" /tmp/pr-$PR.diff | grep \"^+\"\r\n\r\n# Missing await (accidentally sequential promises)\r\ngrep -n \"await.*await\" /tmp/pr-$PR.diff | grep \"^+\" | head -10\r\n\r\n# Large in-memory allocations\r\ngrep -n \"new Array([0-9]\\{4,\\}\\|Buffer\\.alloc\" /tmp/pr-$PR.diff | grep \"^+\"\r\n```\r\n\r\n---\r\n\r\n## Ticket Linking Verification\r\n\r\n```bash\r\n# Extract ticket references from PR body\r\ngh pr view $PR --json body | jq -r '.body' | \\\r\n  grep -oE \"(PROJ-[0-9]+|[A-Z]+-[0-9]+|https://linear\\.app/[^)\\\"]+)\" | sort -u\r\n\r\n# Verify Jira ticket exists (requires JIRA_API_TOKEN to be SET in the environment).\r\n# Credentials are fed to curl via a config read from stdin (-K -) so the token\r\n# never appears in argv — `ps aux` / /proc/*/cmdline can't see it, and nothing\r\n# secret lands in shell history. Never paste the raw token on the command line.\r\nTICKET=\"PROJ-123\"\r\n: \"${JIRA_API_TOKEN:?JIRA_API_TOKEN must be set}\"\r\ncurl -s -K - \"https://your-org.atlassian.net/rest/api/3/issue/$TICKET\" <<EOF | \\\r\n  jq '{key, summary: .fields.summary, status: .fields.status.name}'\r\nuser = \"user@company.com:$JIRA_API_TOKEN\"\r\nEOF\r\n\r\n# Linear ticket — same pattern: the Authorization header goes through the\r\n# stdin config, not a -H flag, to keep the key out of the process list.\r\nLINEAR_ID=\"abc-123\"\r\n: \"${LINEAR_API_KEY:?LINEAR_API_KEY must be set}\"\r\ncurl -s -K - -H \"Content-Type: application/json\" \\\r\n  --data \"{\\\"query\\\": \\\"{ issue(id: \\\\\\\"$LINEAR_ID\\\\\\\") { title state { name } } }\\\"}\" \\\r\n  https://api.linear.app/graphql <<EOF | jq .\r\nheader = \"Authorization: $LINEAR_API_KEY\"\r\nEOF\r\n```\r\n\r\n> **Security note:** for repeated Jira use, prefer a `~/.netrc` entry\r\n> (`machine your-org.atlassian.net login user@company.com password <token>`,\r\n> `chmod 600 ~/.netrc`) and call `curl -s --netrc …` — no secret material in\r\n> the command at all.\r\n\r\n---\r\n\r\n## Complete Review Checklist (30+ Items)\r\n\r\n```markdown\r\n## Code Review Checklist\r\n\r\n### Scope & Context\r\n- [ ] PR title accurately describes the change\r\n- [ ] PR description explains WHY, not just WHAT\r\n- [ ] Linked Jira/Linear ticket exists and matches scope\r\n- [ ] No unrelated changes (scope creep)\r\n- [ ] Breaking changes documented in PR body\r\n\r\n### Blast Radius\r\n- [ ] Identified all files importing changed modules\r\n- [ ] Cross-service dependencies checked\r\n- [ ] Shared types/interfaces/schemas reviewed for breakage\r\n- [ ] New env vars documented in .env.example\r\n- [ ] DB migrations are reversible (have down() / rollback)\r\n\r\n### Security\r\n- [ ] No hardcoded secrets or API keys\r\n- [ ] SQL queries use parameterized inputs (no string interpolation)\r\n- [ ] User inputs validated/sanitized before use\r\n- [ ] Auth/authorization checks on all new endpoints\r\n- [ ] No XSS vectors (innerHTML, dangerouslySetInnerHTML)\r\n- [ ] New dependencies checked for known CVEs\r\n- [ ] No sensitive data in logs (PII, tokens, passwords)\r\n- [ ] File uploads validated (type, size, content-type)\r\n- [ ] CORS configured correctly for new endpoints\r\n\r\n### Testing\r\n- [ ] New public functions have unit tests\r\n- [ ] Edge cases covered (empty, null, max values)\r\n- [ ] Error paths tested (not just happy path)\r\n- [ ] Integration tests for API endpoint changes\r\n- [ ] No tests deleted without clear reason\r\n- [ ] Test names clearly describe what they verify\r\n\r\n### Breaking Changes\r\n- [ ] No API endpoints removed without deprecation notice\r\n- [ ] No required fields added to existing API responses\r\n- [ ] No DB columns removed without two-phase migration plan\r\n- [ ] No env vars removed that may be set in production\r\n- [ ] Backward-compatible for external API consumers\r\n\r\n### Performance\r\n- [ ] No N+1 query patterns introduced\r\n- [ ] DB indexes added for new query patterns\r\n- [ ] No unbounded loops on potentially large datasets\r\n- [ ] No heavy new dependencies without justification\r\n- [ ] Async operations correctly awaited\r\n- [ ] Caching considered for expensive repeated operations\r\n\r\n### Code Quality\r\n- [ ] No dead code or unused imports\r\n- [ ] Error handling present (no bare empty catch blocks)\r\n- [ ] Consistent with existing patterns and conventions\r\n- [ ] Complex logic has explanatory comments\r\n- [ ] No unresolved TODOs (or tracked in ticket)\r\n```\r\n\r\n---\r\n\r\n## Output Format\r\n\r\nStructure your review comment as:\r\n\r\n```\r\n## PR Review: [PR Title] (#NUMBER)\r\n\r\nBlast Radius: HIGH — changes lib/auth used by 5 services\r\nSecurity: 1 finding (medium severity)\r\nTests: Coverage delta +2%\r\nBreaking Changes: None detected\r\n\r\n--- MUST FIX (Blocking) ---\r\n\r\n1. SQL Injection risk in src/db/users.ts:42\r\n   Raw string interpolation in WHERE clause.\r\n   Fix: db.query(\"SELECT * WHERE id = $1\", [userId])\r\n\r\n--- SHOULD FIX (Non-blocking) ---\r\n\r\n2. Missing auth check on POST /api/admin/reset\r\n   No role verification before destructive operation.\r\n\r\n--- SUGGESTIONS ---\r\n\r\n3. N+1 pattern in src/services/reports.ts:88\r\n   findUser() called inside results.map() — batch with findManyUsers(ids)\r\n\r\n--- LOOKS GOOD ---\r\n- Test coverage for new auth flow is thorough\r\n- DB migration has proper down() rollback method\r\n- Error handling consistent with rest of codebase\r\n```\r\n\r\n---\r\n\r\n## Common Pitfalls\r\n\r\n- **Reviewing style over substance** — let the linter handle style; focus on logic, security, correctness\r\n- **Missing blast radius** — a 5-line change in a shared utility can break 20 services\r\n- **Approving untested happy paths** — always verify error paths have coverage\r\n- **Ignoring migration risk** — NOT NULL additions need a default or two-phase migration\r\n- **Indirect secret exposure** — secrets in error messages/logs, not just hardcoded values\r\n- **Skipping large PRs** — if a PR is too large to review properly, request it be split\r\n\r\n---\r\n\r\n## Best Practices\r\n\r\n1. Read the linked ticket before looking at code — context prevents false positives\r\n2. Check CI status before reviewing — don't review code that fails to build\r\n3. Prioritize blast radius and security over style\r\n4. Reproduce locally for non-trivial auth or performance changes\r\n5. Label each comment clearly: \"nit:\", \"must:\", \"question:\", \"suggestion:\"\r\n6. Batch all comments in one review round — don't trickle feedback\r\n7. Acknowledge good patterns, not just problems — specific praise improves culture"
  user_template: ""
variables: []
install:
  steps:
    - title: Download & unzip
      detail: >-
        Download the .zip package from SkillVault and unzip it. The pr-review-expert/ folder
        contains SKILL.md plus any companion scripts, references and assets.
    - title: Copy into your agent skills directory
      detail: >-
        Move the folder into your agent's skills directory — e.g. ~/.claude/skills/ (Claude Code),
        .codex/skills/, .cursor/skills/ or your project's .claude/skills/ folder — then restart or
        reload the session.
    - title: Use it
      detail: >-
        Describe the task in natural language; the agent loads the skill automatically when the
        request matches the skill description.
usage:
  steps: []
metadata:
  author: SkillVault
  source: https://skillvault.top/skills/pr-review-expert
  exported_at: "2026-11-03T07:45:00.412Z"