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

Step 2: If Pattern/Architecture → Cross-Reference Skills

Only check skills if the suggestion involves patterns we have rules for:

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

• • What category is this? (Bug fix / Docs / Code quality / Pattern)
• • If pattern change → check relevant skills
• • Does it conflict with project conventions?
• • Provide clear AGREE / DISAGREE / PARTIAL response