# Pr Review

> Review pull request for code quality, security, performance, and best practices. Use when reviewing PRs, checking code before merge, or providing feedback.

- Skill: `allanninal/pr-review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add allanninal/pr-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/allanninal/pr-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Security
- Author: allanninal (https://skillmd.com/u/allanninal)
- Updated: 2026-09-10
- Page: https://skillmd.com/skills/allanninal/pr-review

---


# Pull Request Reviewer

Comprehensive PR review covering code quality, security, performance, and best practices.

## Arguments
- `$ARGUMENTS`: PR number or branch name to review

## PR Context

Fetch PR information first:

```bash
# Get PR details
gh pr view $ARGUMENTS --json title,body,author,additions,deletions,files,commits

# Get PR diff
gh pr diff $ARGUMENTS

# Get changed files list
gh pr diff $ARGUMENTS --name-only

# Get PR comments
gh pr view $ARGUMENTS --comments
```

## Review Checklist

### 1. Code Quality

- [ ] **Readability**: Is the code easy to understand?
- [ ] **Naming**: Are variables, functions, and classes named clearly?
- [ ] **DRY**: Is there code duplication that should be refactored?
- [ ] **Complexity**: Are functions/methods reasonably sized?
- [ ] **Comments**: Are complex sections documented?
- [ ] **Dead Code**: Are there unused imports, variables, or functions?

### 2. Security Review

**Check for:**
- [ ] SQL/NoSQL injection vulnerabilities
- [ ] XSS vulnerabilities (unsanitized user input in HTML)
- [ ] Authentication/authorization bypasses
- [ ] Hardcoded secrets or API keys
- [ ] Insecure dependencies
- [ ] Improper error handling (exposing stack traces)
- [ ] Missing input validation

**Patterns to search:**
```bash
# SQL injection
grep -rn "execute.*\$\|format.*%s\|f\".*{.*}.*WHERE" --include="*.py"

# Hardcoded secrets
grep -rn "api_key\|secret\|password\|token" --include="*.py" --include="*.ts" --include="*.js"

# Unsafe eval
grep -rn "eval\|exec\|dangerouslySetInnerHTML"

# XSS patterns
grep -rn "innerHTML\|document.write\|\.html\("
```

### 3. Performance Review

- [ ] **N+1 Queries**: Are database queries efficient?
- [ ] **Caching**: Is appropriate caching used?
- [ ] **Lazy Loading**: Are large resources loaded lazily?
- [ ] **Memory Leaks**: Are resources properly cleaned up?
- [ ] **Async Operations**: Are I/O operations non-blocking?

**Check MongoDB queries (Beanie):**
```python
# Good: Using select_related equivalent
await Model.find().limit(100).to_list()

# Bad: Loading all in memory
await Model.find().to_list()  # No limit!
```

### 4. Testing Review

- [ ] **Coverage**: Are new features covered by tests?
- [ ] **Edge Cases**: Are edge cases tested?
- [ ] **Mocking**: Are external dependencies properly mocked?
- [ ] **Test Names**: Do test names describe what they test?

### 5. Architecture Review

- [ ] **Separation of Concerns**: Is business logic separate from HTTP handling?
- [ ] **Dependency Injection**: Are dependencies properly injected?
- [ ] **Error Handling**: Are errors handled consistently?
- [ ] **API Design**: Are API endpoints RESTful and consistent?

### 6. Documentation Review

- [ ] **README**: Is README updated if needed?
- [ ] **API Docs**: Are new endpoints documented?
- [ ] **Comments**: Are complex algorithms explained?
- [ ] **CHANGELOG**: Is changelog updated?

## Project-Specific Checks

### FastAPI Projects (eruditiontx, mathmatterstx)

```python
# Check route patterns
# Good
@router.get("/items/{item_id}")
async def get_item(item_id: str, current_user: User = Depends(get_current_user)):

# Check for proper validation
# Good: Using Pydantic models
async def create_item(item: ItemCreate):

# Check role-based access
# Good: Using JWTBearer dependency
@router.post("/admin/action", dependencies=[Depends(JWTBearer(access_levels=["admin"]))])
```

### Next.js Projects (notaryo.ph, bocs-turbo)

```typescript
// Check for proper server/client separation
// Good: 'use server' for server actions
'use server'
export async function createItem() {}

// Check for proper error boundaries
// Good: error.tsx in route folders

// Check for proper loading states
// Good: loading.tsx in route folders
```

## Output Format

```markdown
## PR Review: #[number] - [title]

### Summary
[Brief description of what this PR does]

### Changes Overview
- Files changed: X
- Additions: +Y
- Deletions: -Z

### Review Findings

#### Code Quality
- [Finding 1 with file:line reference]
- [Finding 2]

#### Security
- [Security finding or "No security issues found"]

#### Performance
- [Performance finding or "No performance concerns"]

#### Testing
- [Testing coverage assessment]

### Recommendations

**Required Changes:**
1. [Critical issue that must be fixed]

**Suggested Improvements:**
1. [Nice-to-have improvement]

### Verdict
- [ ] Approve
- [ ] Request Changes
- [ ] Comment
```

## Automated Checks

Before review, run automated checks:

```bash
# Lint the changed files
/lint-fix $(gh pr diff $ARGUMENTS --name-only | tr '\n' ' ')

# Run tests
/test all

# Check for security vulnerabilities
npm audit  # or pip-audit for Python
```