| name | code-review-evaluation |
| description | Evaluate external PR suggestions (Gemini, CodeRabbit) against project conventions. Use when asked to review or agree with external code review comments. |
Code Review Evaluation Skill
Purpose
Evaluate external code review suggestions (Gemini, CodeRabbit, etc.) against project conventions. These tools have access to our codebase and often make valid suggestions - but they don't know our project-specific conventions.
What External Reviewers ARE Good At
CodeRabbit, Gemini, etc. can see your code and are excellent at catching:
- ✅ Missing fields in examples/docs (like missing
geoScope in docstring)
- ✅ Type mismatches and inconsistencies
- ✅ Unused imports/variables
- ✅ Potential null/undefined errors
- ✅ Security vulnerabilities (generic patterns)
- ✅ Documentation gaps
- ✅ Test coverage suggestions
- ✅ General best practices (accessibility, performance)
These suggestions are usually VALID - agree with them!
What External Reviewers DON'T Know
They lack context about our project-specific conventions:
- ❌ Our
@i18n/routing Link requirement
- ❌ Our
/types directory governance
- ❌ Our three-layer API proxy pattern
- ❌ Our design system semantic classes
- ❌ Our env variable 4-location rule
- ❌ Our $300 searchParams incident history
Only check skills when suggestion involves these patterns.
Evaluation Framework
When user asks "Do you agree with this suggestion?":
Step 1: Categorize the Suggestion
| Category | Examples | Default Response |
|---|
| Bug/Error fix | Missing field, type error, null check | Usually AGREE |
| Documentation | Missing JSDoc, outdated example | Usually AGREE |
| Code quality | Unused code, simplification | Usually AGREE |
| Code complexity | >100 lines/function, >10 props, >3 nesting | Usually AGREE |
| Pattern change | Different import, new abstraction | CHECK SKILLS |
| Architecture | Component structure, API calls | CHECK SKILLS |
Step 2: If Pattern/Architecture → Cross-Reference Skills
Only check skills if the suggestion involves patterns we have rules for:
| Skill | Check For |
|---|
pre-implementation-checklist | Does suggestion duplicate existing code? |
type-system-governance | Does it create types outside /types? |
react-nextjs-patterns | Does it violate server-first pattern? |
api-layer-patterns | Does it bypass three-layer proxy? |
url-canonicalization | Does it add searchParams to listing pages? |
i18n-best-practices | Does it use next/link instead of @i18n/routing? |
design-system-conventions | Does it use gray-* or arbitrary utilities? |
env-variable-management | Does it add env var without all 5 locations? |
data-validation-patterns | Does it skip Zod validation? |
security-headers-csp | Does it use raw fetch() without wrappers? |
Step 3: Provide Structured Response
Response Template
## Evaluation: [Suggestion Title/Topic]
### Summary
[1-2 sentence summary of the suggestion]
### Category
[Bug fix / Documentation / Code quality / Pattern change / Architecture]
### Skill Check (if pattern/architecture)
- [ ] [Relevant skill] - [Pass/Conflict: reason]
### Recommendation
**[AGREE / DISAGREE / PARTIAL]**
[Explanation]
Example: Valid Suggestion (AGREE)
CodeRabbit says: "Docstring example missing geoScope field"
## Evaluation: Missing geoScope in docstring
### Summary
Example in JSDoc is missing a required field from the type.
### Category
Documentation - external reviewer caught a real inconsistency.
### Skill Check
Not needed - this is a documentation fix, not a pattern change.
### Recommendation
**AGREE** - The docstring example should match the actual type definition.
Project-Specific Blind Spots (CHECK SKILLS)
Only disagree when suggestion conflicts with our conventions:
1. "Use next/link directly"
Our rule: Always use Link from @i18n/routing
Response: Disagree - breaks locale handling
2. "Add types inline in component"
Our rule: All types in /types directory
Response: Disagree - violates type governance
3. "Use gray-500 for text"
Our rule: Use semantic tokens (text-foreground/80)
Response: Disagree - violates design system
4. "Add searchParams to page component for filtering"
Our rule: NEVER in listing pages (causes $300+ cost spikes)
Response: Disagree - violates url-canonicalization (CRITICAL)
5. "Use raw fetch() for this API call"
Our rule: Use fetchWithHmac or safeFetch
Response: Disagree - violates security patterns
6. "Create a helper function here"
Our rule: Search for existing patterns first
Response: Partial - check if pattern exists before creating
7. "Use axios instead of fetch"
Our rule: Use fetchWithHmac (internal) or safeFetch (external)
Response: Disagree - we have standardized fetch wrappers
8. "Prefer @types/* path alias over bare types/ path"
Our rule: Use types/* not @types/*
Response: Disagree - @types/* conflicts with TypeScript's convention for type definition packages (e.g., @types/react, @types/node). We intentionally use types/* to avoid this collision.
Quick Decision Tree
Is the suggestion about...?
│
├─ Bug/error fix → Usually AGREE ✅
├─ Missing docs/fields → Usually AGREE ✅
├─ Unused code cleanup → Usually AGREE ✅
├─ Test coverage → Usually AGREE ✅
│
├─ Types/interfaces → Check type-system-governance
├─ Components → Check react-nextjs-patterns
├─ Styling → Check design-system-conventions
├─ API calls → Check api-layer-patterns
├─ URLs/routing → Check url-canonicalization
├─ Links → Check i18n-best-practices
├─ Env vars → Check env-variable-management
├─ Validation → Check data-validation-patterns
├─ Security → Check security-headers-csp
├─ New utility → Check pre-implementation-checklist
More Examples
Example: Missing Field in Docstring (AGREE)
Suggestion: "Docstring example missing geoScope field"
Response: AGREE - External reviewer correctly identified inconsistency between example and type.
Example: Suggestion to use useMemo (PARTIAL)
Suggestion: "Wrap this computation in useMemo for performance"
Response: PARTIAL - Only if profiling shows actual bottleneck. Don't prematurely optimize.
Example: Suggestion to inline styles (DISAGREE)
Suggestion: "Use inline className with Tailwind utilities"
Response: DISAGREE - design-system-conventions requires semantic classes.
When to AGREE (Most Cases)
External reviewers are usually right about:
- Missing fields in docs/examples
- Type inconsistencies
- Unused imports/variables
- Null/undefined safety
- Documentation improvements
- Test coverage gaps
- Accessibility issues
- General security issues
When to DISAGREE (Project-Specific)
Only disagree when suggestion conflicts with our skills:
- Uses
next/link instead of @i18n/routing
- Creates types outside
/types
- Uses
gray-* instead of semantic tokens
- Adds
searchParams to listing pages
- Uses raw
fetch() instead of our wrappers
- Bypasses three-layer API pattern
- Adds env var without 4-location update
Checklist
