antoniocascais/pr-review

PR Review Skill

Reviews code changes with focus on quality, security, and consistency.

Default Assumption: Public Repository

Unless explicitly stated otherwise, assume the repository is publicly available. This means:

• Any secret, credential, or API key pushed is considered compromised
• Internal URLs, IPs, hostnames should not be exposed
• Comments with sensitive internal context should be flagged
• Error messages should not leak internal architecture
• Be extra cautious with .env files, config files, CI/CD configs

Phase 1: Determine Scope

STOP. Use AskUserQuestion before anything else.

Ask user to choose review scope:

• Staged files only
• Unstaged changes (working directory)
• All uncommitted (staged + unstaged)
• Current branch vs main (PR-style)
• Specific commit or range
• Other (specify)

Do NOT run any git commands or tools until user responds.

After selection, get the diff:

• Staged: git diff --cached
• Unstaged: git diff
• All uncommitted: git diff HEAD
• Branch vs main: git diff main...HEAD
• Commit: git show <hash>
• Range: git diff <from>..<to>

Also get changed files list: git diff --name-only <appropriate args>

Phase 2: Understand the Problem

STOP. Use AskUserQuestion to confirm before proceeding.

Infer intent from:

1. Branch name: git branch --show-current
2. Commit messages: git log main..HEAD --oneline (or relevant range)

Then use AskUserQuestion to confirm:

"Based on branch feature/xyz and commits, this PR appears to [inferred description]. Is this correct?"

• Yes, proceed
• No, let me explain

Do NOT proceed until user confirms.

Phase 3: Auto-Detect Stack

Check for presence of:

• package.json / yarn.lock → Node.js
• requirements.txt / pyproject.toml → Python
• go.mod → Go
• Cargo.toml → Rust
• Dockerfile → Docker
• *.tf → Terraform
• *.yaml in k8s patterns → Kubernetes
• .github/workflows/ → GitHub Actions

Note detected stack for context-aware analysis.

Phase 4: Run Scanners

Execute relevant scanners (skip silently if not installed):

Always run: | Tool | Command | |------|---------| | gitleaks | gitleaks detect --source . --verbose --no-git | | trufflehog | trufflehog filesystem . --only-verified |

Stack-specific: | Stack | Tool | Command | |-------|------|---------| | Node.js | npm audit | npm audit --json | | Node.js | yarn audit | yarn audit --json | | Python | pip-audit | pip-audit | | Python | safety | safety check | | Docker | trivy | trivy fs . | | Docker | hadolint | hadolint Dockerfile | | Terraform | tfsec | tfsec . | | Terraform | checkov | checkov -d . | | Terraform | trivy | trivy config . | | K8s | trivy | trivy config . | | Shell scripts | shellcheck | shellcheck <file> |

Phase 5: Code Review

Analyze the diff for all categories. Be pragmatic—flag likely issues, skip obvious false positives.

5.1 Code Quality

• Best practices for detected stack
• Readability and maintainability
• Error handling appropriateness
• Test coverage (if tests exist)
• Idiomatic patterns
• Type safety issues

5.2 Codebase Consistency

• Match existing patterns in the repo
• Naming conventions alignment
• File organization consistency
• Don't introduce a 10th way of doing something

5.3 Security

Manual checks:

• Hardcoded secrets, API keys, passwords, connection strings
• SQL injection, XSS, command injection vectors
• Path traversal risks
• Auth/authz bypasses
• Insecure defaults (http vs https, weak crypto)
• Sensitive data in logs/errors/URLs
• Container: running as root, privileged mode, unverified base images

5.4 Bug Detection

• Logic errors, off-by-one
• Null/undefined handling
• Race conditions
• Resource leaks (unclosed handles, connections)
• Breaking changes to existing APIs

5.5 Dependencies

• Known vulnerable package versions
• Outdated dependencies with security patches
• Unpinned versions
• Suspicious or typosquatted package names

5.6 Performance

• N+1 query patterns
• Sync operations in async contexts
• Unbounded loops/recursion
• Memory leaks
• Missing pagination
• Blocking I/O in hot paths

5.7 Deprecations & Drift

• Deprecated APIs, functions, patterns
• Breaking changes in dependencies
• Hardcoded values that should be variables
• Environment-specific configs in shared code
• Configuration diverging from IaC patterns

Phase 6: Report

Output a succinct markdown report:

## PR Review: [brief title]

**Problem:** [1-2 sentences on what this PR solves]

**Scope:** [staged/branch/commits reviewed]

**Stack:** [detected tech stack]

### Scanner Results
| Tool | Result |
|------|--------|
| gitleaks | [clean/N findings] |
| ... | ... |

### Findings

#### CRITICAL
- `file:line` - [issue with brief context]

#### HIGH
- `file:line` - [issue]

#### MEDIUM
- `file:line` - [issue]

#### LOW
- `file:line` - [issue]

### Summary
- Critical: X | High: X | Medium: X | Low: X

### Review Score: X/20
[One sentence justification]

### Action Required
| Priority | Item |
|----------|------|
| blocker | ... |
| should fix | ... |
| consider | ... |

Rating Scale

| Score | Meaning | Action | |-------|---------|--------| | 0-10 | Blocker issues | Reject, needs significant rework | | 11-15 | Acceptable | Merge after addressing fixes | | 16-17 | Good | Ready to merge, suggestions optional | | 18-20 | Excellent | Merge immediately |

Style Guidelines

Keep findings concise but contextual:

• Bad: "should use https here"

• Good: "http exposes data in transit, use https"

• Bad: "fix this null check"

• Good: "user.email accessed without null check - crashes if user not found"

Don't write a 50-page report. Focus on what matters.