pr-review — independently scanned and version-tracked by SaferSkills.
SaferSkills independently audited pr-review (Agent Skill) and scored it 100/100 (green). The audit ran 55 deterministic rules across Security, Supply Chain, Maintenance, Transparency, and Community; it found 0 high-severity and 0 lower-severity findings. The full rule-by-rule trace and per-finding evidence are below. Free, methodology-open.
Findings & checks · 0 flagged
Every scanned point with the score it earned and what moved between them.
First recorded scan — no prior version to compare against.
The primary manifest — the file an agent reads to learn what this artifact does.
Systematic, evidence-based pull request review with anti-sycophancy principles. Critique-first ordering ensures critical issues are identified before any positive observations.
Core principle: Review what changed, not what it's called. Labels lie. Tests verify author's assumptions—review catches flawed assumptions.
Violating the letter of this process is violating the spirit of code review.
Always use for:
Lighter review acceptable for:
Never skip:
NO APPROVAL WITHOUT COMPLETING ALL CRITICAL DIMENSIONSSecurity, Correctness, and Test Coverage must ALWAYS be reviewed. No exceptions.
Severity source: TheCritical/High/Medium/Lowlevels used below are the Code Review Severity scale defined in../../rules/severity-classification.md. This skill decides which dimensions to review at each level; the canonical file defines what counts as each level. Do not conflate these levels with production incident severity (P1-P4).
| Priority | Action | Dimensions |
|---|---|---|
| Critical | Must complete ALWAYS | Correctness, Security, Tests |
| High | Must complete for non-trivial PRs | Architecture, Code Quality |
| Medium | Complete based on change type | Performance, Accessibility |
| Low | Complete if time permits | Documentation |
| Issue | Why It Blocks |
|---|---|
| Hardcoded secrets | Security vulnerability |
| SQL injection possible | Data breach risk |
| Missing auth on protected route | Unauthorized access |
| No tests for new code | Regression risk |
| Critical security issue | Production incident |
| Finding | Decision |
|---|---|
| Any Critical issue | Request Changes |
| Multiple High issues | Request Changes |
| Only Medium/Low issues | Approve with Comments |
| No issues, all Critical dimensions reviewed | Approve |
Efficient reviews are thorough reviews. Time pressure causes shortcuts that miss issues.
| PR Size | Target Time | Max Time | If Exceeded |
|---|---|---|---|
| XS (1-10 lines) | 5-10 min | 15 min | Document blockers |
| S (11-50 lines) | 15-30 min | 45 min | Request clarification |
| M (51-200 lines) | 30-60 min | 90 min | Split focus areas |
| L (201-500 lines) | 60-120 min | 180 min | Request smaller PR |
| XL (500+ lines) | Request split | - | Don't review as-is |
Time exceeded? You're either:
| Anti-Pattern | Why It Wastes Time | Instead |
|---|---|---|
| Reviewing file-by-file | Misses cross-file relationships | Review by logical change |
| Style nitpicking first | Depletes attention on low-value | Critical dimensions first |
| Rewriting in comments | You're doing their work | Describe what's wrong; brief suggestions OK (see Constructive Feedback) |
| Reviewing draft PRs | Premature feedback changes | Wait for "Ready for Review" |
| Multiple review passes | Tiring for both parties | One thorough pass |
Immediately request split if:
How to request:
This PR combines [X] and [Y]. Could you split into separate PRs? Review will be faster and more thorough for each.
Different PR types require different focus areas. Use these guides to prioritize your review.
| Focus | Why | Key Checks |
|---|---|---|
| Requirements alignment | New code must solve the right problem | Does implementation match ticket/spec? |
| API design | New interfaces are hard to change | Clean contracts? Versioning considered? |
| Test coverage | New behavior needs verification | Happy path + edge cases + errors tested? |
| Performance baseline | New features set expectations | Acceptable load? Indexed queries? |
Time allocation: 40% correctness, 30% architecture, 20% tests, 10% docs
| Focus | Why | Key Checks |
|---|---|---|
| Root cause | Fixes that mask problems return | Does fix address cause, not symptom? |
| Regression test | Same bug shouldn't reappear | Test proves bug existed AND is fixed? |
| Related code | Similar bugs often cluster | Other code with same pattern? |
| Minimal change | Bug fixes shouldn't refactor | Only changes necessary for fix? |
Time allocation: 50% correctness, 30% tests, 15% impact radius, 5% docs
| Focus | Why | Key Checks |
|---|---|---|
| Behavior preservation | Refactoring ≠ new behavior | Tests unchanged? Same inputs → same outputs? |
| Incremental steps | Big refactors hide bugs | Each commit independently valid? |
| No feature creep | "While I'm here..." is a trap | Scope matches stated goal? |
| Test coverage before/after | Can't verify what you can't test | Coverage same or better? |
Time allocation: 50% behavior preservation, 30% architecture, 15% tests, 5% docs
| Focus | Why | Key Checks |
|---|---|---|
| Security advisories | Main reason for updates | CVE addressed? New vulnerabilities? |
| Breaking changes | Changelog often buried | Migration guide followed? |
| Transitive deps | Your dep's deps matter | Lock file changes reviewed? |
| Build verification | Updates can break builds | CI passing? Bundle size acceptable? |
Time allocation: 40% security, 30% breaking changes, 20% build verification, 10% changelog
| Focus | Why | Key Checks |
|---|---|---|
| Contract compatibility | Callers depend on stable interfaces | Backward compatible? Deprecation path? |
| Versioning strategy | Breaking changes need coordination | Version bump appropriate? |
| Documentation | API users need to know what changed | Changelog updated? Migration guide? |
| Consumer impact | Changes ripple through dependents | Who calls this? Are they updated? |
Time allocation: 40% compatibility, 25% consumer impact, 20% documentation, 15% versioning
digraph review_process {
rankdir=TB;
node [shape=box];
start [label="PR Received" shape=ellipse];
context [label="1. Gather Context\n(description, ticket, scope)"];
change_type [label="2. Identify Change Type\n(feature/bugfix/refactor)" shape=diamond];
security_scan [label="3. Security Quick Scan\n(4 critical items)" style=filled fillcolor="#ffcccc"];
critical_dim [label="4. Critical Dimensions\n(Correctness, Tests)" style=filled fillcolor="#ffcccc"];
high_dim [label="5. High Dimensions\n(Architecture, Quality)"];
medium_dim [label="6. Medium/Low Dimensions\n(Performance, A11y, Docs)"];
devils_advocate [label="7. Devil's Advocate\n(What could go wrong?)"];
impact_radius [label="8. Impact Radius\n(Dependencies, contracts)"];
decision [label="9. Make Decision" shape=diamond];
approve [label="Approve" style=filled fillcolor="#ccffcc"];
request_changes [label="Request Changes" style=filled fillcolor="#ffcccc"];
start -> context;
context -> change_type;
change_type -> security_scan;
security_scan -> critical_dim;
critical_dim -> high_dim;
high_dim -> medium_dim;
medium_dim -> devils_advocate;
devils_advocate -> impact_radius;
impact_radius -> decision;
decision -> approve [label="No Critical/High"];
decision -> request_changes [label="Critical/High found"];
}Key: Red boxes = mandatory, never skip.
Does the code do what it claims to do?
| ID | Check | Priority |
|---|---|---|
| CR-001 | Logic matches stated requirements/ticket | Critical |
| CR-002 | Edge cases handled (null, empty, boundary) | Critical |
| CR-003 | Error conditions handled appropriately | Critical |
| CR-004 | Data transformations preserve all fields | High |
| CR-005 | Conditional branches cover all cases | High |
| CR-006 | Off-by-one errors checked in loops | High |
| CR-007 | Async operations await properly | High |
| CR-008 | State mutations intentional and controlled | Medium |
See Security Quick Scan section for 4 critical items (~2 min).
Full checklist for deeper review:
| ID | Check | Priority |
|---|---|---|
| SEC-001 | No hardcoded secrets/credentials | Critical |
| SEC-002 | SQL queries parameterized | Critical |
| SEC-003 | Auth required on protected routes | Critical |
| SEC-004 | Authorization verifies ownership | Critical |
| SEC-005 | XSS prevented (output encoding) | High |
| SEC-006 | CSRF tokens on state changes | High |
| SEC-007 | Sensitive data not logged | High |
| SEC-008 | File uploads validated | High |
| ID | Check | Priority |
|---|---|---|
| TEST-001 | New code has corresponding tests | Critical |
| TEST-002 | Tests verify behavior, not implementation | High |
| TEST-003 | Edge cases and error paths tested | High |
| TEST-004 | Tests are readable (clear names) | Medium |
| TEST-005 | No mocking of implementation details | Medium |
| TEST-006 | Coverage meets target (90%+ core) | High |
| TEST-007 | Tests run successfully (no flaky) | Critical |
| TEST-008 | Integration tests for cross-component | Medium |
| ID | Check | Priority |
|---|---|---|
| ARCH-001 | Layer boundaries respected | High |
| ARCH-002 | Dependency direction correct | High |
| ARCH-003 | Single Responsibility followed | High |
| ARCH-004 | No premature abstraction (YAGNI) | Medium |
| ARCH-005 | Changes localized (not scattered) | Medium |
| ARCH-006 | Public APIs designed for stability | High |
| ARCH-007 | Appropriate patterns (not over-engineered) | Medium |
| ARCH-008 | Migration path for breaking changes | High |
| ID | Check | Priority |
|---|---|---|
| CQ-001 | No code duplication (DRY) | High |
| CQ-002 | Functions small (10-20 lines) | Medium |
| CQ-003 | Cyclomatic complexity acceptable (<=10) | Medium |
| CQ-004 | Nesting depth reasonable (<=3) | Medium |
| CQ-005 | TypeScript types explicit (no any) | High |
| CQ-006 | Names meaningful and consistent | High |
| CQ-007 | Dead code removed | Medium |
| CQ-008 | Comments explain 'why' not 'what' | Low |
| ID | Check | Priority |
|---|---|---|
| PERF-001 | No N+1 queries | High |
| PERF-002 | Expensive operations memoized | Medium |
| PERF-003 | Bundle size impact acceptable | Medium |
| PERF-004 | No blocking on main thread | High |
| PERF-005 | Large lists virtualized | Medium |
| PERF-006 | Images optimized | Low |
| PERF-007 | Caching strategy appropriate | Medium |
| PERF-008 | Memory leaks prevented | High |
| ID | Check | Priority |
|---|---|---|
| A11Y-001 | Semantic HTML used | High |
| A11Y-002 | Keyboard accessible | High |
| A11Y-003 | Images have alt text | Medium |
| A11Y-004 | Form inputs have labels | High |
| A11Y-005 | Color not only indicator | Medium |
| A11Y-006 | Focus management correct | High |
| A11Y-007 | ARIA attributes correct | Medium |
| A11Y-008 | Screen reader verified | Medium |
| ID | Check | Priority |
|---|---|---|
| DOC-001 | PR description explains 'why' | Medium |
| DOC-002 | Complex logic commented | Low |
| DOC-003 | Public APIs documented | Medium |
| DOC-004 | README updated if needed | Low |
| DOC-005 | Breaking changes documented | High |
| DOC-006 | Config changes documented | Medium |
| DOC-007 | Migration guide provided | Medium |
| DOC-008 | Commit messages clear | Low |
Always run. Takes ~2 minutes. Catches 80% of critical security issues.
| ID | Check | Detection Pattern |
|---|---|---|
| SEC-001 | No hardcoded secrets | Search: password=, apiKey=, token=, secret= |
| SEC-002 | SQL injection prevented | Look for string concatenation in queries |
| SEC-003 | Auth on protected routes | New routes have auth middleware |
| SEC-004 | Authorization checks | Resource access verifies ownership |
If ANY fail → STOP. Request changes immediately.
Run for UI changes. Takes ~2 minutes. Catches common A11Y barriers.
| ID | Check | Detection Pattern |
|---|---|---|
| A11Y-001 | Semantic HTML used | Search: <div onClick=, <span onClick= (should be buttons) |
| A11Y-002 | Keyboard accessible | onClick handlers without onKeyDown/onKeyPress |
| A11Y-004 | Form inputs have labels | <input> without associated label or aria-label |
| A11Y-006 | Focus management correct | tabIndex manipulation, focus traps in modals |
If ANY fail in user-facing UI → Request changes.
Mandatory section. Challenge the implementation.
| Question | How to Check |
|---|---|
| What imports this file? | grep -r "import.*from.*<file>" |
| What calls this function? | Find references in IDE |
| What extends this class? | Search for extends <class> |
| Change Type | Breaking? | Action |
|---|---|---|
| Removed export | Yes | Find all usages, update |
| Changed parameter | Likely | Check all callers |
| Narrowed return type | Likely | Verify consumers handle |
| Added required field | Yes | Update all creators |
| Changed behavior | Maybe | Verify tests cover |
For refactoring PRs: Tests passing ≠ Bug-free. Manual verification required.
| Area | Question | ||
|---|---|---|---|
| Conditional branches | Does each if/else/switch branch work as intended? | ||
| Data transformations | Do all fields map correctly? | ||
| Optional handling | Do ?., ??, `\ | \ | ` provide intended fallbacks? |
| Dependency injection | Are all dependencies properly wired? | ||
| Edge cases | Null, undefined, empty array handled? |
Output format:
file.ts:42 - [issue]: Missing null check on user.address| Don't Say | Why |
|---|---|
| "Great job" | Subjective praise |
| "Well done" | Subjective praise |
| "Excellent work" | Subjective praise |
| "Looks good to me" (without evidence) | Empty approval |
| "LGTM" (without review) | Rubber stamp |
| Instead Of | Use |
|---|---|
| "Great error handling" | "Error handling covers: network failures, validation errors, timeouts" |
| "Nice architecture" | "Architecture follows layer separation: API → Service → Repository" |
| "Good tests" | "Tests cover: happy path, 3 edge cases, 2 error conditions" |
Every observation must include:
file.ts:lineAnti-sycophancy prevents false praise. This section ensures feedback that's both honest AND helpful.
Note: Brief suggestions are fine ("Consider using X"). Avoid rewriting entire implementations in comments—that's doing their work for them (see Efficiency Anti-Patterns).
| Tone | Example | When to Use |
|---|---|---|
| Blocking | "This will cause data loss. Must fix before merge." | Critical issues, security, data integrity |
| Requesting | "Please add null check here—crash possible." | High issues requiring change |
| Suggesting | "Consider extracting this to a helper—readability." | Medium issues, optional improvements |
| Noting | "FYI: Similar pattern in utils/helpers.ts" | Knowledge sharing, not requiring change |
| Questioning | "What happens if X is empty? I might be missing context." | Uncertain findings, seeking clarification |
| Instead Of | Use |
|---|---|
| "This is wrong" | "This will cause [specific problem] because [reason]" |
| "Why did you do this?" | "What was the reasoning for [approach]? I want to understand the context." |
| "This doesn't make sense" | "I'm having trouble following [specific part]. Could you add a comment?" |
| "You should have..." | "For future reference, [pattern] helps with [benefit]" |
| "Obviously..." | Remove. If it were obvious, it wouldn't need saying. |
| "Just do X" | "One option: [X]. This would [benefit]." |
For junior developers:
For senior developers:
When author pushes back on feedback:
Never: Make it personal. Attack the code, not the coder.
Section order is mandatory. Critique-first.
## PR Review: [PR Title]
### 1. Context
- Change type: [feature/bugfix/refactor]
- Scope: [X files, Y lines added, Z removed]
- Linked: [ticket/issue]
### 2. Critical Findings
| Issue | Location | Impact | Severity |
|-------|----------|--------|----------|
| [finding] | file:line | [impact] | Critical/High |
### 3. Security Quick Scan
- [x] No hardcoded secrets
- [x] SQL injection prevented
- [ ] Auth on protected routes - MISSING on `/api/users`
- [x] Authorization checks
### 4. Devil's Advocate
- **Risk**: [what could go wrong]
- **Assumption**: [what might be wrong]
- **Edge case**: [unhandled scenario]
### 5. Impact Radius
- Direct dependencies: [list]
- Contract changes: [breaking/non-breaking]
### 6. Dimension Review
[Per-dimension findings with file:line references]
### 7. Decision
**Request Changes** / **Approve** / **Approve with Comments**
Reason: [evidence-based justification]| Excuse | Reality |
|---|---|
| "This change is too simple" | Simple changes break things too. Checklist takes 2 min. |
| "The tests pass" | Tests verify author's assumptions. Review catches flawed assumptions. |
| "I trust this developer" | Trust ≠ verification. Everyone makes mistakes. |
| "Security isn't my expertise" | Basic checklist doesn't require expertise. Follow it. |
| "Accessibility is nice-to-have" | Accessibility is a requirement. It's in the checklist. |
| "I'll do quick now, thorough later" | 'Later' never comes. Approved code ships. |
| "The PR is too large" | Large PR = high risk. Request smaller PRs or more time. |
| "It's just a hotfix" | Hotfixes become permanent. Technical debt accrues interest. |
| "Already reviewed similar code" | Context matters. This PR has different state. |
| "Internal tooling, doesn't matter" | Internal tools become external. Bad patterns spread. |
| "Generated code is safe" | Generated code can have bad templates. Review it. |
| "Just dependency updates" | Dependencies are code you didn't write. Security review MORE important. |
If you catch yourself thinking any of these, STOP and return to checklist:
ALL of these mean: STOP. Return to checklist. Complete all Critical dimensions.
| Problem | Solution |
|---|---|
| PR too large | Request smaller PRs. If urgent, review in chunks by component. |
| Unfamiliar domain | Ask author for context. Review related PRs. Check docs/ADRs. |
| Conflicting requirements | Escalate to product owner. Document assumptions in review. |
| Time pressure | Complete Critical dimensions ONLY. Note skipped areas explicitly. |
| Author pushback | Focus on objective evidence. Reference standards/checklist IDs. |
| Can't reproduce issue | Request reproduction steps. Check environment differences. |
| Uncertain about impact | Use Impact Radius Analysis. When in doubt, request changes. |
| Missing context | Check linked ticket/issue. Ask author before assuming. |
If still stuck: Escalate to specialist (see Specialist Escalation section).
Before submitting your review:
Cannot check all boxes? Do not submit approval.
Reviews are learning opportunities. Capture and propagate valuable patterns.
| Discovery | Action |
|---|---|
| Reusable pattern found | Add to team's pattern library or wiki |
| Common mistake identified | Create linting rule or add to onboarding docs |
| Unclear API discovered | Request/add documentation in that PR |
| Performance insight gained | Share in team channel with benchmark |
| Security vulnerability pattern | Add to security checklist, alert security team |
In the PR itself:
Beyond the PR:
After each review, ask:
Track your own growth:
Measure review effectiveness to improve the process.
| Metric | How to Measure | Target |
|---|---|---|
| Review turnaround | Time from "Ready" to first review | < 4 hours |
| Review cycles | Number of review → change → review loops | ≤ 2 |
| Escapees | Bugs found in prod that reviews missed | 0 |
| False positives | Requested changes that weren't actually issues | < 10% |
| Review coverage | PRs merged without review | 0% |
Tag findings for pattern analysis:
| Category | Examples |
|---|---|
security | Auth bypass, injection, secrets |
correctness | Logic errors, missing cases |
performance | N+1, memory leaks, blocking |
maintainability | Complexity, duplication, naming |
testing | Missing tests, flaky tests |
documentation | Missing/outdated docs |
Weekly:
Monthly:
Quarterly:
Don't:
| Platform | Metric Source | Notes |
|---|---|---|
| GitHub | Insights → Pulse | Review activity, cycle time |
| GitHub | GraphQL API | Custom queries for turnaround |
| GitLab | Analytics → Code Review | Built-in review metrics |
| Bitbucket | Reports → Commits | PR activity tracking |
| Third-party | LinearB, Sleuth, Haystack | Cross-platform analytics |
CLI quick checks:
# GitHub: Recent PR review times (requires gh CLI)
gh pr list --state merged --json number,mergedAt,createdAt --limit 20
# Git: Author-reviewer pairs (local analysis)
git log --format='%an reviewed by %cn' --merges --since='1 month ago'For deep-dive reviews, reference specialist agents:
| Domain | Agent |
|---|---|
| Security | .ai-rules/agents/security-specialist.json |
| Performance | .ai-rules/agents/performance-specialist.json |
| Accessibility | .ai-rules/agents/accessibility-specialist.json |
| Architecture | .ai-rules/agents/architecture-specialist.json |
| Test Strategy | .ai-rules/agents/test-strategy-specialist.json |
| Code Quality | .ai-rules/agents/code-quality-specialist.json |
~30 seconds. Free. No account. Every finding cites a rule and a line of evidence.