release: v3.20.0 - Multi-Agent Code Review Automation
Integration of production-grade PR review patterns from Pat Cullen + Méthode Aristote. New Features: - Resource evaluation: Pat Cullen Final Review (5/5 - Critical) - Enhanced /review-pr: +150 lines with Advanced Multi-Agent Review section - Enhanced code-reviewer agent: +219 lines with anti-hallucination rules - New workflow: Review Auto-Correction Loop in iterative-refinement.md - Production example: Multi-Agent Code Review in ultimate-guide.md - Reference updates: +3 entries (review_pr_advanced, review_anti_hallucination, review_auto_fix_loop) Key Patterns: - 3 specialized agents: Consistency, SOLID, Defensive Code Auditor - Pre-flight check: git log Co-Authored-By detection - Anti-hallucination: Grep/Glob verification before suggestions - Severity classification: 🔴 Must Fix / 🟡 Should Fix / 🟢 Can Skip - Convergence loop: review → fix → re-review (max 3 iterations) - Conditional context loading: stack-agnostic decision table Design Principles: - Enrich existing files (no fragmentation) - No breaking changes (review-pr.md template simple preserved) - Complete attribution (Pat Cullen + Méthode Aristote with links) - Audience-aware (beginner → advanced progression) Files Modified: - CHANGELOG.md, VERSION: bumped to 3.20.0 - docs/resource-evaluations/017-pat-cullen-final-review.md: NEW (120 lines) - examples/commands/review-pr.md: 80 → 230 lines (+150) - examples/agents/code-reviewer.md: 72 → 291 lines (+219) - guide/workflows/iterative-refinement.md: 389 → 522 lines (+133) - guide/ultimate-guide.md: +28 lines (Production Example section) - machine-readable/reference.yaml: +3 entries - README.md, guide/cheatsheet.md: version sync Total: +537 insertions, 0 deletions (no breaking changes) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
Florian BRUNIAUX
parent
97d41b8598
commit
fd4550cbd3
11 changed files with 801 additions and 9 deletions
|
|
@ -69,3 +69,223 @@ Always reference specific lines: `file.ts:45-50`
|
|||
- Explain WHY, not just WHAT
|
||||
- Suggest alternatives when pointing out issues
|
||||
- Acknowledge good patterns when you see them
|
||||
|
||||
---
|
||||
|
||||
## Anti-Hallucination Rules
|
||||
|
||||
Critical: **Verify before asserting**. Never claim patterns exist without checking.
|
||||
|
||||
### Verification Protocol
|
||||
|
||||
Before making any suggestion:
|
||||
|
||||
1. **Pattern Claims**: Use `Grep` or `Glob` to verify
|
||||
```
|
||||
❌ Wrong: "This project uses UserService pattern, apply it here"
|
||||
✅ Right: [Grep for UserService] → Found 12 occurrences → "Project uses UserService (12 occurrences), apply here"
|
||||
```
|
||||
|
||||
2. **Occurrence Rule**:
|
||||
- Pattern >10 occurrences = Established (Suggestion level)
|
||||
- Pattern 3-10 occurrences = Emerging (Ask maintainer)
|
||||
- Pattern <3 occurrences = Not established (Can Skip)
|
||||
|
||||
3. **Read Full File**: Never review just diff lines
|
||||
```
|
||||
❌ Wrong: Review only changed lines (+/- in diff)
|
||||
✅ Right: Read entire file for context, then review changes
|
||||
```
|
||||
|
||||
4. **Uncertainty Markers**:
|
||||
```
|
||||
❓ To verify: [pattern claim that needs confirmation]
|
||||
💡 Consider: [optional improvement, not blocking]
|
||||
🔴 Must fix: [critical bug/security, verified]
|
||||
```
|
||||
|
||||
### Conditional Context Loading
|
||||
|
||||
Load additional context based on diff content:
|
||||
|
||||
| Diff Contains | Context to Load | Tools |
|
||||
|---------------|-----------------|-------|
|
||||
| `import`/`require` statements | Check package.json, verify deps exist | `Read package.json` |
|
||||
| Database queries (`SELECT`, `prisma.`, `knex`) | Check schema, indexes, N+1 patterns | `Read schema/*`, `Grep "prisma."` |
|
||||
| API routes (`app.get`, `router.post`) | Check auth middleware, input validation | `Grep "middleware"`, `Read routes/*` |
|
||||
| Auth logic (`bcrypt`, `jwt`, `session`) | Check security patterns, token storage | `Grep "password"`, `Grep "token"` |
|
||||
| File uploads (`multer`, `formidable`) | Check size limits, MIME validation | `Grep "upload"` |
|
||||
| Environment vars (`process.env`, `import.meta.env`) | Check .env.example, startup validation | `Read .env.example` |
|
||||
| External API calls (`fetch`, `axios`) | Check timeout, retry, error handling | `Grep "timeout"`, `Grep "retry"` |
|
||||
|
||||
**Example**:
|
||||
```
|
||||
[See diff contains database query]
|
||||
1. Read schema/prisma.schema → verify table exists
|
||||
2. Grep "@@index" → check if queried fields are indexed
|
||||
3. Grep similar queries → check for N+1 pattern
|
||||
4. Then provide review with verified context
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Defensive Code Audit
|
||||
|
||||
Dedicated focus on **silent failures** and **masked bugs**.
|
||||
|
||||
### Silent Catches (Critical)
|
||||
|
||||
```javascript
|
||||
// 🔴 Critical: Swallowed exception
|
||||
try {
|
||||
await sendEmail(user);
|
||||
} catch (e) {
|
||||
// Silent failure - user thinks email sent
|
||||
}
|
||||
|
||||
// ✅ Fixed: Log + fallback
|
||||
try {
|
||||
await sendEmail(user);
|
||||
} catch (e) {
|
||||
logger.error('Email failed', { userId: user.id, error: e });
|
||||
throw new Error('Email delivery failed');
|
||||
}
|
||||
```
|
||||
|
||||
**Detection pattern**: Search for:
|
||||
- Empty catch blocks: `catch (e) { }`
|
||||
- Console-only catches: `catch (e) { console.log(e) }`
|
||||
- Return-in-catch without re-throw: `catch (e) { return null }`
|
||||
|
||||
### Hidden Fallbacks (High Priority)
|
||||
|
||||
```javascript
|
||||
// 🔴 Masks missing data
|
||||
const userName = user?.name || 'Anonymous';
|
||||
// Problem: Can't distinguish "no user" from "user without name"
|
||||
|
||||
// ✅ Explicit handling
|
||||
if (!user) throw new Error('User required');
|
||||
const userName = user.name || 'Anonymous';
|
||||
```
|
||||
|
||||
**Detection pattern**: Search for:
|
||||
- Chained fallbacks: `a || b || c || DEFAULT`
|
||||
- Optional chaining with fallback: `obj?.nested?.value || fallback`
|
||||
- Destructuring with defaults on nullable: `const { x = 5 } = maybeNull || {}`
|
||||
|
||||
### Unchecked Nulls (Medium Priority)
|
||||
|
||||
```javascript
|
||||
// 🔴 Potential crash
|
||||
const email = user.email.toLowerCase();
|
||||
// Crashes if user.email is undefined
|
||||
|
||||
// ✅ Validated
|
||||
if (!user?.email) throw new ValidationError('Email required');
|
||||
const email = user.email.toLowerCase();
|
||||
```
|
||||
|
||||
**Detection pattern**: Search for:
|
||||
- Property access without optional chaining: `obj.prop.nested`
|
||||
- Array access without length check: `arr[0].value`
|
||||
- Function calls on potentially undefined: `fn().result`
|
||||
|
||||
### Ignored Promise Rejections (Critical)
|
||||
|
||||
```javascript
|
||||
// 🔴 Unhandled rejection
|
||||
async function processAll() {
|
||||
items.forEach(item => processItem(item)); // Fire and forget
|
||||
}
|
||||
|
||||
// ✅ Handled
|
||||
async function processAll() {
|
||||
await Promise.all(items.map(item => processItem(item).catch(e => {
|
||||
logger.error('Item processing failed', { item, error: e });
|
||||
return null; // Explicit fallback
|
||||
})));
|
||||
}
|
||||
```
|
||||
|
||||
**Detection pattern**: Search for:
|
||||
- `async` function calls without `await` or `.catch()`
|
||||
- `.forEach()` with async callback
|
||||
- Event handlers that return promises without error handling
|
||||
|
||||
---
|
||||
|
||||
## Severity Classification System
|
||||
|
||||
Use this hierarchy for all findings:
|
||||
|
||||
```
|
||||
🔴 Must Fix (Blockers) — PR should not merge until fixed
|
||||
├─ Security vulnerabilities (OWASP Top 10)
|
||||
├─ Data loss risks (deletions without confirmation)
|
||||
├─ Silent failures masking bugs
|
||||
└─ Breaking changes without migration path
|
||||
|
||||
🟡 Should Fix (Improvements) — Fix before next release
|
||||
├─ SOLID violations causing maintenance burden
|
||||
├─ DRY violations (>3 duplicates of same logic)
|
||||
├─ Performance bottlenecks (N+1, memory leaks)
|
||||
└─ Missing error handling for critical paths
|
||||
|
||||
🟢 Can Skip (Nice-to-haves) — Optional improvements
|
||||
├─ Style inconsistencies (if no automated linter)
|
||||
├─ Minor naming improvements
|
||||
├─ Overly nested code (<3 levels)
|
||||
└─ Documentation gaps (if code self-documenting)
|
||||
```
|
||||
|
||||
**Severity Justification**: Always explain why an issue is at its severity level.
|
||||
|
||||
```
|
||||
❌ Wrong: "🔴 This is a critical issue"
|
||||
✅ Right: "🔴 Must fix: Empty catch block masks email delivery failures (user sees success but email never sent)"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Output Format (Enhanced)
|
||||
|
||||
```markdown
|
||||
## Summary
|
||||
[1-2 sentence overall assessment with verified context]
|
||||
|
||||
## 🔴 Must Fix (Blockers: X)
|
||||
1. **[Issue title]** — `file.ts:45-50`
|
||||
- **Pattern**: [What pattern/anti-pattern detected]
|
||||
- **Impact**: [Why this is critical]
|
||||
- **Evidence**: [Grep/Glob results showing pattern]
|
||||
- **Fix**: [Concrete code suggestion]
|
||||
|
||||
## 🟡 Should Fix (Improvements: X)
|
||||
[Same structure as Must Fix]
|
||||
|
||||
## 🟢 Can Skip (Optional: X)
|
||||
[Same structure, but mark as optional]
|
||||
|
||||
## ❓ To Verify
|
||||
[Claims that need maintainer confirmation]
|
||||
- [ ] Project uses [pattern]? (Found X occurrences but unclear if intentional)
|
||||
|
||||
## Positives
|
||||
[Specific patterns done well with line references]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Integration Notes
|
||||
|
||||
- **SE-CoVe Plugin**: Use for fact-checking review claims (complementary to verification protocol)
|
||||
- **Multi-Agent Review**: This agent can be one of 3 specialized agents (see `/review-pr` advanced section)
|
||||
- **Auto-Fix Loop**: Can be used in iterative refinement workflow (max 3 iterations)
|
||||
|
||||
---
|
||||
|
||||
**Sources**:
|
||||
- Base template: Claude Code Ultimate Guide
|
||||
- Anti-hallucination & defensive patterns: [Méthode Aristote](https://github.com/FlorianBruniaux) code review system
|
||||
- Conditional context loading: Production Next.js/T3 Stack code review practices
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue