Code Review Skill v3 (Requirement-Driven)

---
name: Code Review Skill v3 (Requirement-Driven)
version: 3.0
description: >
  Reusable 6-phase requirement-driven code review logic. Validates that PR/MR changes
  implement JIRA requirements, with comprehensive code quality analysis, scoring, and
  grading. Tech-agnostic patterns for requirement validation, code quality review,
  test coverage analysis, documentation analysis, and scorecard calculation.
applies_to: [java, python, react, mssql, all-languages]
---

# Code Review Skill v3 — Requirement-Driven Analysis

---

## 1. Overview

**Code Review Skill v3** provides reusable logic for requirement-driven code reviews. Instead of reviewing code in isolation, it validates that PR/MR changes actually implement what was requested in the JIRA ticket.

### Key Innovation

Combines **requirement validation** (new) + **code quality review** (existing) in a single framework with transparent scoring and grading.

### Process Phases

| Phase | Input | Output | Scoring |
|-------|-------|--------|---------|
| 1. Requirement Analysis | JIRA ticket | Layman's terms summary + ACs | N/A (qualitative) |
| 2. Requirement Validation | Code diff + Requirement | Coverage %, gap list | 0-100% |
| 3. Code Quality Review | Code diff | Issues by category/severity | 0-100% per category |
| 4. Test Coverage Analysis | Code + test files | Coverage %, missing scenarios | 0-100% |
| 5. Documentation Analysis | Code diff | Doc completeness %, gaps | 0-100% |
| 6. Scorecard Calculation | All % scores | Final grade A-F | Weighted average |

---

## 2. Phase 1: Requirement Analysis

### Purpose
Parse the JIRA requirement and translate it to plain English. Identify acceptance criteria and scope boundaries.

### Input
- JIRA ticket object:
  ```json
  {
    "key": "PROJ-123",
    "summary": "User Registration with Email Validation",
    "description": "Allow new users to sign up with email and password...",
    "acceptance_criteria": [
      "User can enter email and password",
      "Email validation is enforced",
      "Passwords are hashed securely",
      "Success notification sent"
    ],
    "scope_notes": "Desktop and mobile friendly"
  }
  ```

### Process Steps

**Step 1: Extract Core Components**
- Feature summary (what is being built?)
- Feature description (why is it needed?)
- Acceptance criteria (AC1, AC2, ...)
- Scope boundaries (what's included? what's NOT?)
- Constraints (performance, security, UX)

**Step 2: Translate to Layman's Terms**
- Remove technical jargon
- Highlight "what success looks like"
- Simplify for non-technical stakeholders
- Note any assumptions or open questions

**Step 3: Identify Key Requirements**
- User-facing changes
- System behavior changes
- Data integrity requirements
- Security/compliance requirements

### Output Format

```markdown
## Requirement Analysis

### Feature
User Registration with Email Validation

### In Plain English
"Allow new users to sign up with email and password. The system should:
- Let users enter their email and password
- Validate the email format
- Hash passwords securely before storage
- Send a confirmation email after signup
- Work on mobile and desktop"

### Acceptance Criteria
- [AC1] User can enter email and password on signup form
- [AC2] Email validation enforces valid format (RFC 5322)
- [AC3] Passwords are hashed with bcrypt (or equiv) before storage
- [AC4] Success confirmation email is sent after signup
- [AC5] Form is responsive on mobile and desktop

### Scope Boundaries
- **Included**: Email/password signup, validation, hashing, email sending
- **NOT Included**: Social login, 2FA, password reset, email verification link
- **Constraints**: Email must be sent within 2 seconds; password rules enforced

### Key Assumptions
- Email service is already available (assumed from JIRA)
- Frontend validation should mirror backend validation
- No rate limiting required at this stage
```

---

## 3. Phase 2: Requirement Validation

### Purpose
Map code changes to acceptance criteria. Score how well the PR implements the requirement.

### Input
- Code diff (files added/modified)
- Requirement from Phase 1

### Process Steps

**Step 1: Map Code to Requirements**
- Which files implement which AC?
- Is each AC addressed in the code?
- Any over-engineering (unnecessary features)?

For the user registration example:
```
AC1 (email/password form)
  ✅ frontend/components/SignupForm.tsx (email, password inputs)
  ✅ backend/routes/auth.py (POST /auth/signup endpoint)

AC2 (email validation)
  ✅ backend/validators/email.py (RFC 5322 validator)
  ✅ frontend/validators/email.ts (client-side check)

AC3 (password hashing)
  ✅ backend/services/auth.py (bcrypt hashing in create_user)

AC4 (confirmation email)
  ✅ backend/services/email.py (send_confirmation_email function)
  ⚠️  No test for email sending failure

AC5 (responsive UI)
  ✅ frontend/styles/signup.css (mobile breakpoints)
  ⚠️  No explicit mobile test
```

**Step 2: Identify Gaps**
- Missing implementations
- Partial implementations
- Untested functionality
- Known workarounds

**Step 3: Calculate Coverage Score**

| Coverage Range | Meaning | Examples |
|---|---|---|
| 100% | All AC fully implemented, no gaps | All AC working, tested, documented |
| 90-99% | All AC implemented, minor gaps | One AC partially tested; docs incomplete |
| 75-89% | Most AC implemented, notable gaps | One AC missing; error handling incomplete |
| 60-74% | Significant AC missing | Two ACs not implemented; basic structure only |
| <60% | Major implementation incomplete | Scope fundamentally incomplete |

### Output Format

```markdown
## Requirement Validation

### Acceptance Criteria Coverage

| AC | Requirement | Status | Notes |
|----|---|---|---|
| AC1 | User can enter email and password | ✅ Complete | Form UI + backend endpoint implemented |
| AC2 | Email validation enforced | ✅ Complete | RFC 5322 validator in backend + frontend |
| AC3 | Passwords hashed securely | ✅ Complete | bcrypt with salt in create_user() |
| AC4 | Confirmation email sent | ⚠️ Partial | Email service integrated, but no error handling |
| AC5 | Responsive UI (mobile + desktop) | ✅ Complete | CSS breakpoints verified |

### Coverage Assessment
- **Requirement Met: 95%**
- Fully implemented: 4/5 AC (AC1, AC2, AC3, AC5)
- Partially implemented: 1/5 AC (AC4 — missing error handling)

### Identified Gaps
1. **AC4 Gap**: Email sending has no retry logic if SMTP fails
   - Impact: Silent failure; user thinks signup worked but never receives email
   - Fix: Add exponential retry + fallback notification

2. **Testing Gap**: No test for email failure scenario
   - Impact: Deployment risk if email service goes down
   - Fix: Mock email service failures in integration tests

### Gap Severity
- P0: Missing implementation of core AC
- P1: Partial implementation (happy path works, error handling missing)
- P2: Untested functionality
- P3: Documentation missing

### Scope Creep Check
- ✅ No unnecessary features added
- ✅ All code aligns with stated ACs
- ✅ No gold-plating detected
```

---

## 4. Phase 3: Code Quality Review

### Purpose
Evaluate code against design, SOLID, patterns, performance, security, testing, and documentation standards.

### Input
- Code diff (full changes)
- Tech stack (detected from file extensions, imports, frameworks)

### Code Quality Categories

#### 4.1 Design & Architecture (Structure)

**Checklist:**
- [ ] Single Responsibility Principle: Each class/function has one reason to change?
- [ ] Appropriate abstraction levels (not god classes, not over-fragmented)?
- [ ] Clear separation of concerns (UI ≠ business logic ≠ data access)?
- [ ] No circular dependencies or tightly coupled modules?
- [ ] Interfaces/contracts defined for key components?

**Scoring Guidance:**
- 90-100%: Clear structure, good separation, extensible
- 75-89%: Minor coupling issues, mostly clean
- 60-74%: Some god classes or tight coupling
- <60%: Structural issues impact maintainability

**Example Issues:**

```
Issue: God Class (Authentication.java)
Severity: P1 (Design issue)
Location: src/auth/Authentication.java, line 1-450
Problem: Single class handles login, logout, token refresh, password hashing, email sending
Why: Hard to test, hard to maintain, violates SRP
Fix: Split into:
  - LoginService (login logic)
  - TokenService (token refresh)
  - PasswordService (hashing)
  - EmailService (sending)
```

#### 4.2 SOLID Principles

**Checklist:**
- [ ] S — Single Responsibility: Each class/function has one reason to change?
- [ ] O — Open/Closed: Open for extension, closed for modification?
- [ ] L — Liskov Substitution: Subclasses can replace parents without breaking?
- [ ] I — Interface Segregation: No client forced to depend on methods it doesn't use?
- [ ] D — Dependency Inversion: Depend on abstractions, not concrete implementations?

**Scoring Guidance:**
- 90-100%: Clear SOLID adherence, good abstractions
- 75-89%: Minor violations (e.g., tight coupling in one module)
- 60-74%: Multiple violations affecting code quality
- <60%: Pervasive SOLID violations, hard to extend

**Example Issues:**

```
Issue: Tight Coupling (PaymentController depends on concrete UserRepository)
Severity: P2 (SOLID violation)
Location: src/controllers/PaymentController.java, line 23
Problem: Constructor: new UserRepository() — hard-coded dependency
Why: Can't swap implementations for testing, violates Dependency Inversion
Fix: Inject UserRepository interface via constructor
```

#### 4.3 Patterns & Best Practices

**Checklist:**
- [ ] Are common patterns used correctly? (Factory, Strategy, Observer, etc.)
- [ ] Are error handling patterns consistent?
- [ ] Are null/empty checks done uniformly?
- [ ] Are async operations handled correctly?
- [ ] Is there code duplication that suggests a missing pattern?

**Scoring Guidance:**
- 90-100%: Good use of patterns, minimal duplication, consistent style
- 75-89%: Some code duplication, minor pattern misuse
- 60-74%: Duplication or anti-patterns in critical paths
- <60%: Pervasive code smells, missed optimization opportunities

**Example Issues:**

```
Issue: Missing Error Pattern (try/except with no handler)
Severity: P1 (Pattern violation)
Location: src/services/payment_service.py, line 42
Problem: try: process_payment() except Exception: pass
Why: Errors silently swallowed; no logging or recovery
Fix: Catch specific exceptions, log, and re-raise or handle gracefully
```

#### 4.4 Performance & Scalability

**Checklist:**
- [ ] No N+1 query patterns (loop with DB call inside)?
- [ ] Pagination used for large datasets?
- [ ] Caching applied to expensive operations?
- [ ] Async/non-blocking for I/O (network, files, DB)?
- [ ] No unnecessary full table scans?
- [ ] No unbounded loops or recursion?
- [ ] Memory usage reasonable (no leaks, unbounded collections)?

**Scoring Guidance:**
- 90-100%: Efficient code, good use of caching, async patterns
- 75-89%: Minor inefficiencies, mostly acceptable performance
- 60-74%: Obvious optimization opportunities (N+1, missing pagination)
- <60%: Code will struggle at scale (full scans, unbounded loops)

**Example Issues:**

```
Issue: N+1 Query Pattern
Severity: P1 (Performance)
Location: src/services/order_service.py, line 67-72
Problem:
  orders = db.query(Order).all()  # Loads all orders
  for order in orders:
    customer = db.query(Customer).filter(...).one()  # N queries!
Why: Will scale poorly; 1000 orders = 1001 queries
Fix: Use JOIN to load customers with orders in single query
  orders = db.query(Order).join(Customer).all()
```

#### 4.5 Security

**Checklist:**
- [ ] No hardcoded secrets (passwords, API keys, tokens)?
- [ ] SQL queries parameterized (no string concatenation)?
- [ ] User inputs validated and sanitized?
- [ ] Authentication/authorization checks present?
- [ ] No sensitive data logged (passwords, tokens, PII)?
- [ ] No insecure deserialization?
- [ ] HTTPS enforced (if applicable)?
- [ ] CORS configured appropriately?

**Scoring Guidance:**
- 90-100%: Strong security posture, validated inputs, no secrets
- 75-89%: Minor issues (e.g., one unvalidated field)
- 60-74%: Multiple security gaps (hardcoded secrets, missing validation)
- <60%: Critical vulnerabilities (SQL injection, exposed credentials)

**Example Issues:**

```
Issue: SQL Injection Vulnerability
Severity: P0 (Security)
Location: src/repositories/user_repo.java, line 34
Problem:
  String query = "SELECT * FROM users WHERE email = '" + email + "'";
  ResultSet rs = statement.executeQuery(query);
Why: Email input not escaped; attacker can inject SQL
Risk: Unauthorized data access, data deletion
Fix: Use parameterized query
  String query = "SELECT * FROM users WHERE email = ?";
  PreparedStatement stmt = connection.prepareStatement(query);
  stmt.setString(1, email);
```

#### 4.6 Testing & Test Coverage

**Checklist:**
- [ ] Unit tests for business logic?
- [ ] Happy path tests present?
- [ ] Error cases tested?
- [ ] Edge cases covered?
- [ ] Test code is clean and maintainable?
- [ ] Mock/stub dependencies appropriately?
- [ ] Coverage >= 80% for critical code?

**Scoring Guidance:**
- 90-100%: >90% coverage, good test names, comprehensive scenarios
- 75-89%: 70-90% coverage, mostly good tests, minor gaps
- 60-74%: <70% coverage, gaps in error cases or edge cases
- <60%: Minimal tests, critical scenarios untested

**Example Issues:**

```
Issue: Missing Error Case Test
Severity: P1 (Testing)
Location: tests/test_payment.py (missing)
Problem: No test for payment failure scenario
Why: Code path untested; might fail silently in production
Fix: Add test_payment_failure_with_retry test case
```

#### 4.7 Documentation & Code Comments

**Checklist:**
- [ ] Public functions have docstrings (with parameters, returns)?
- [ ] Complex logic has inline comments?
- [ ] Edge cases documented?
- [ ] API/contract examples provided?
- [ ] README or architecture docs present?

**Scoring Guidance:**
- 90-100%: Comprehensive docstrings, clear examples, architecture documented
- 75-89%: Most functions documented, minor gaps
- 60-74%: Some functions undocumented, complex logic unclear
- <60%: Minimal documentation, hard to understand

**Example Issues:**

```
Issue: Missing Docstring
Severity: P2 (Documentation)
Location: src/services/payment_service.py, line 45
Problem: validate_card() function has no docstring
Why: Unclear what parameters are expected, what exceptions can be raised
Fix: Add docstring:
  """
  Validate credit card details.
  
  Args:
    card_number (str): 16-digit card number
    expiry (str): Expiry date as MM/YY
    cvv (str): 3-digit CVV code
    
  Returns:
    bool: True if valid, raises CardValidationError if invalid
    
  Raises:
    CardValidationError: If card details invalid
  """
```

### Code Quality Scoring Summary

Calculate a percentage for each category:

```markdown
## Code Quality Assessment

| Category | Score | Status | Notes |
|----------|-------|--------|-------|
| Design & Structure | 85% | Good | Minor coupling in auth module |
| SOLID Principles | 90% | Good | Good abstraction, dependency injection used |
| Patterns & Practices | 80% | Fair | Some code duplication in validators |
| Performance | 95% | Excellent | Efficient queries, good caching |
| Security | 100% | Excellent | Parameterized queries, no hardcoded secrets |
| Testing | 70% | Fair | Happy path covered, error cases missing |
| Documentation | 75% | Fair | Most functions documented, edge cases unclear |

### Overall Code Quality Score: **85%**
= Average of all categories (rounded to nearest 5%)
```

---

## 5. Phase 4: Test Coverage Analysis

### Purpose
Analyze tests to verify that code changes are adequately tested. Check for happy path, error cases, and edge cases.

### Input
- Code diff (added/modified production code)
- Test files (new/modified test code)

### Process Steps

**Step 1: Quantify Code Changes**
```
Production Code:
- Lines added: 150
- Lines modified: 45
- Lines deleted: 20

Test Code:
- Lines added: 120
- Lines modified: 30
- Lines deleted: 5
```

**Step 2: Identify Test Scenarios**
For each new/modified function, check for:

| Scenario | Example | Status |
|----------|---------|--------|
| Happy path | Valid input → correct output | ✅ Should test |
| Error cases | Invalid input → exception | ⚠️ Often missing |
| Boundary cases | Empty, null, negative, max values | ⚠️ Often missing |
| Integration | Multiple functions together | ⚠️ May need integration test |
| Concurrency | Simultaneous requests | ⚠️ For async code |

**Step 3: Coverage Analysis**

```
Function: create_user(email, password)
  Total lines: 25
  
  Happy Path Test: test_create_user_valid_email
    ✅ Covers: valid email input, password hashing, user creation
    Lines tested: 15/25 (60%)
  
  Error Cases:
    ⚠️ MISSING: test_create_user_invalid_email
    ⚠️ MISSING: test_create_user_duplicate_email
    ⚠️ MISSING: test_create_user_db_failure
  
  Estimated Coverage: 60%
```

**Step 4: Calculate Test Coverage Score**

| Coverage % | Meaning | Rating |
|---|---|---|
| 90-100% | Comprehensive: happy + errors + edges | Excellent |
| 75-89% | Good: happy path + some error cases | Good |
| 60-74% | Fair: happy path mostly covered | Fair |
| 40-59% | Weak: very basic testing | Weak |
| <40% | Critical: barely tested | Critical Gap |

### Output Format

```markdown
## Test Coverage Analysis

### Code Coverage Metrics
- Production code added: 215 lines
- Production code modified: 45 lines
- Test code added: 120 lines
- Estimated overall coverage: 75%

### Test Scenario Checklist

#### Function: POST /auth/signup (backend/routes/auth.py)
- [✅] Happy path: Valid email + password → user created
- [✅] Error case: Invalid email → 400 Bad Request
- [⚠️] Error case: Duplicate email → 409 Conflict (not tested)
- [⚠️] Error case: Email service fails → 500 with graceful error
- [✅] Edge case: Very long password (255+ chars)
- [⚠️] Integration: Email sent successfully after signup

#### Function: validateEmail() (backend/validators/email.py)
- [✅] Happy path: valid@example.com → True
- [✅] Error case: invalid-email → False
- [✅] Edge case: email@localhost (no TLD) → False
- [⚠️] Edge case: international domain (IDN) → handling unclear

### Coverage Gaps
1. **Missing Error Case Test**: Duplicate email signup
   - Severity: P1
   - Impact: Concurrent signups might not be handled correctly
   - Fix: Add test_create_user_duplicate_email test

2. **Missing Integration Test**: Email failure recovery
   - Severity: P1
   - Impact: Silent failures if email service is down
   - Fix: Mock email service failure, verify retry logic

3. **Missing Edge Case Test**: Password edge cases
   - Severity: P2
   - Impact: May fail with very long or special characters
   - Fix: Add parameterized test with edge cases

### Test Quality Assessment
- Test naming: ✅ Good (givenX_whenY_thenZ pattern used)
- AAA pattern: ✅ Good (Arrange-Act-Assert clear)
- Mocking: ✅ Good (dependencies properly mocked)
- Assertions: ✅ Clear and specific

### Overall Test Coverage Score: **75%**
= (Lines with test coverage / Total production lines) × 100
```

---

## 6. Phase 5: Documentation Analysis

### Purpose
Verify that code changes are well-documented with docstrings, comments, and examples.

### Input
- Code diff (new/modified functions and classes)

### Process Steps

**Step 1: Check Documentation Completeness**

For each public function/class, verify:

| Item | Example | Required |
|------|---------|----------|
| Function docstring | """Create user and send email""" | ✅ Yes |
| Parameter docs | Args: email (str): User email | ✅ Yes |
| Return docs | Returns: User object | ✅ Yes |
| Exception docs | Raises: ValueError if email invalid | ✅ Yes |
| Usage example | Email will be sent within 2s | ⚠️ For complex APIs |
| Inline comments | # Hash password using bcrypt | ✅ For complex logic |

**Step 2: Assess Documentation Quality**

```
Function: create_user(email: str, password: str) → User

✅ Documented:
  """
  Create a new user account.
  
  Args:
    email (str): User email (RFC 5322 format required)
    password (str): Plain-text password (min 8 chars)
    
  Returns:
    User: Created user object with ID
    
  Raises:
    ValueError: If email invalid or duplicate
    SMTPError: If confirmation email fails
    
  Example:
    user = create_user("alice@example.com", "secure123")
    # User created, confirmation email sent within 2s
  """

❌ Missing:
  - Inline comments explaining password hashing strategy
  - Edge cases documented (what if email service slow?)
```

**Step 3: Calculate Documentation Score**

| Score | Criteria | Examples |
|-------|----------|----------|
| 90-100% | All public functions documented, examples provided | Comprehensive docstrings, clear examples |
| 75-89% | Most functions documented, some gaps | Functions documented, few examples |
| 60-74% | Partial documentation, important gaps | Basic docstrings, no complex logic explained |
| <60% | Minimal documentation | Few docstrings, hard to understand |

### Output Format

```markdown
## Documentation Analysis

### Documentation Checklist

#### Function: POST /auth/signup
- [✅] Function docstring present
- [✅] Parameters documented (email, password)
- [✅] Return value documented (User object)
- [✅] Exceptions documented (ValueError, SMTPError)
- [⚠️] No usage example provided
- [✅] Complex password hashing logic commented

#### Function: validateEmail()
- [✅] Function docstring present
- [✅] Parameters documented
- [✅] Return value documented
- [⚠️] Exception documentation incomplete (what InvalidEmailError contains?)
- [✅] RFC 5322 reference provided
- [✅] Edge cases documented (international domains)

#### Class: AuthService
- [✅] Class docstring present
- [⚠️] No usage example (how to inject into controller?)
- [✅] Constructor parameters documented
- [⚠️] Complex retry logic not fully explained

### Documentation Gaps
1. **Missing Usage Example**: How to handle signup errors in frontend?
   - Type: API documentation
   - Severity: P2
   - Fix: Add example to SignupService docstring

2. **Incomplete Exception Docs**: What is InvalidEmailError?
   - Type: Exception documentation
   - Severity: P2
   - Fix: Document InvalidEmailError class, common cases

3. **Missing Inline Comments**: Password hashing strategy not explained
   - Type: Code comments
   - Severity: P3
   - Fix: Add comment explaining why bcrypt + salt used

### Documentation Quality Score: **80%**
= (Functions with complete docs / Total public functions) × 100

Quality Breakdown:
- Docstrings: 90% complete
- Examples: 40% present (should be >70%)
- Inline comments: 85% (complex logic explained)
- README/architecture: 0% (no top-level docs provided)
```

---

## 7. Phase 6: Scorecard Calculation

### Purpose
Combine all scores from phases 2-5 into a final grade.

### Input
- Requirement Coverage % (Phase 2)
- Code Quality % (Phase 3)
- Test Coverage % (Phase 4)
- Documentation % (Phase 5)

### Scoring Formula

```
Final Score = (Requirement × 0.40) + (CodeQuality × 0.30) + 
              (TestCoverage × 0.20) + (Documentation × 0.10)

Grade Scale:
  A: 90-100  (Excellent — minimal issues)
  B: 80-89   (Good — some improvements needed)
  C: 70-79   (Fair — notable gaps)
  D: 60-69   (Weak — significant issues)
  F: <60     (Failing — not ready to merge)
```

### Worked Example

```
Input Scores:
  - Requirement Met: 95%
  - Code Quality: 85%
  - Test Coverage: 75%
  - Documentation: 80%

Calculation:
  = (95 × 0.40) + (85 × 0.30) + (75 × 0.20) + (80 × 0.10)
  = 38 + 25.5 + 15 + 8
  = 86.5

Grade: B (Good)
Status: ⚠️ Changes Needed — mostly ready, needs test improvements

Justification:
  ✅ Requirement fully implemented (95%)
  ✅ Code well-designed (85%)
  ⚠️ Test coverage needs improvement (75% → should be 90%+)
  ✅ Documentation adequate (80%)
```

### Output Format

```markdown
## Scorecard & Final Grade

### Component Scores
| Component | Score | Weight | Weighted | Status |
|-----------|-------|--------|----------|--------|
| Requirement Met | 95% | 40% | 38.0 | ✅ Excellent |
| Code Quality | 85% | 30% | 25.5 | ✅ Good |
| Test Coverage | 75% | 20% | 15.0 | ⚠️ Fair |
| Documentation | 80% | 10% | 8.0 | ✅ Good |
| **TOTAL** | | | **86.5** | |

### Final Grade: **B** (86.5/100)

### Grade Justification
**Status:** ⚠️ Changes Needed Before Merge

**Strengths:**
- Requirement fully implemented (95%)
- Code well-structured (85%)
- Documentation adequate (80%)
- No security issues found

**Areas for Improvement:**
- Test coverage needs 15% improvement (75% → 90%)
  - Missing error case tests (duplicate email, email failure)
  - Missing edge case tests (long passwords, special chars)

**Recommendation:**
Approve with changes requested:
1. Add missing error case tests (2-3 tests)
2. Add edge case tests for validators
3. Verify email failure scenario is handled

**Estimated effort to fix:** 2-3 hours

### Next Steps
1. Author addresses test coverage gaps
2. Request re-review after changes
3. Once B+ achieved, ready to merge
```

---

## 8. Issue Format & Severity Scale

### Issue Structure

Every issue must include these fields:

```json
{
  "category": "Security|Design|SOLID|Patterns|Performance|Testing|Documentation",
  "severity": "P0|P1|P2|P3",
  "blocks_merge": true,
  "file": "src/auth/register.py",
  "line_range": "42-45",
  "title": "SQL injection vulnerability in user lookup",
  "description": "User input not parameterized in SQL query",
  "why_it_matters": "Attacker can inject SQL to read/modify/delete arbitrary data",
  "before_code": "cur.execute('SELECT * FROM users WHERE id = ' + str(user_id))",
  "after_code": "cur.execute('SELECT * FROM users WHERE id = ?', (user_id,))",
  "references": "OWASP Top 10 #1: Injection",
  "effort_to_fix": "15 minutes"
}
```

**New v3.0 fields:**
- `blocks_merge` (boolean) — whether this issue must be fixed before PR can merge (typically true for P0/P1)
- `before_code` (string) — current/broken code that needs fixing
- `after_code` (string) — corrected code (replaces `suggested_fix`)

### Severity Scale

| Level | Code | Meaning | Action | Block Merge? |
|-------|------|---------|--------|--------------|
| Critical | P0 | Causes data loss, outages, security breaches in production | Fix before next deploy | ✅ Yes |
| High | P1 | Causes user-visible failures or significant perf degradation | Fix in current sprint | ✅ Yes |
| Medium | P2 | Causes intermittent issues or technical debt that compounds | Fix in next sprint | ⚠️ Case-by-case |
| Low | P3 | Code smell, maintainability, minor inefficiency | Fix when touching code | ❌ No |

### Triage Rules & Merge Blocking

- **P0** (`blocks_merge: true`): Always block merge. Examples: SQL injection, unhandled exception, data corruption risk
  - Status in report: 🔴 CRITICAL — FIX REQUIRED BEFORE MERGE
- **P1** (`blocks_merge: true` unless explicitly marked as advisory): Usually block merge. Examples: Missing test, missing doc, poor performance, API design issue
  - Status in report: 🟠 HIGH — RECOMMEND FIX BEFORE MERGE
- **P2** (`blocks_merge: false`): Acceptable with acknowledgment. Examples: Code duplication, magic numbers, suboptimal but working
  - Status in report: 🟡 MEDIUM — FIX IN NEXT SPRINT (optional)
- **P3** (`blocks_merge: false`): Informational. Examples: Style issues, minor refactoring suggestions, linting hints
  - Status in report: 🟢 LOW — NICE-TO-HAVE (FIX LATER)

### Context-Aware Review Rules

When context parameters are provided to quality:review:
- `review-scope`: Only deeply review files matching scope; label others "OUT OF SCOPE — not reviewed"
- `success-criteria`: Treat each criterion as an additional AC to validate in PHASE 2
- `business-justification`: Weight security/compliance issues more heavily when compliance is cited (e.g., SOC 2)
- `context`: Surface in every phase output header so the reviewer understands the WHY

---

## 9. Output Report Structure

### Section 1: Executive Summary

```markdown
## Executive Summary

**PR:** #456 (feature/user-auth → main)  
**JIRA:** PROJ-123 (User Registration with Email Validation)  
**Submitted by:** alice@example.com  
**Review Date:** 2026-05-25 14:30 UTC

### Quick Stats
- Files changed: 8
- Lines added: 350
- Lines removed: 25
- Tests added: 45

### Overall Grade: **B** (86.5/100)

**Status:** ⚠️ Changes Needed — Please address test coverage before merge

**Summary:**
Feature is well-implemented with clean code. Requirement fully met (95%). 
Main gap: test coverage needs improvement (75% → 90%) to cover error cases 
and edge cases. Once tests are added, this will be ready to merge.
```

### Section 2: Requirement Validation

```markdown
## Requirement Validation

### Feature Overview
User Registration with Email Validation

### Plain English
"Allow new users to sign up with email and password. System validates email format, 
hashes passwords securely, and sends a confirmation email."

### Acceptance Criteria Coverage
| AC | Requirement | Status | Notes |
|---|---|---|---|
| AC1 | User can enter email and password | ✅ | Form implemented, backend endpoint ready |
| AC2 | Email validation enforced | ✅ | RFC 5322 validator in place |
| AC3 | Passwords hashed securely | ✅ | bcrypt with salt implemented |
| AC4 | Confirmation email sent | ⚠️ | Email sent, but no retry on failure |
| AC5 | Responsive UI | ✅ | Mobile/desktop tested |

### Requirement Coverage: **95%**

### Gaps Found
1. **AC4 Gap**: Email sending has no retry logic
   - Impact: P1 — If email service temporarily fails, signup appears successful but user never receives confirmation
   - Fix: Add 3-retry logic with exponential backoff
```

### Section 3: Code Quality Issues

```markdown
## Code Quality Issues

### Summary
- **Total Issues:** 4
- **P0 (Critical):** 0
- **P1 (High):** 1
- **P2 (Medium):** 2
- **P3 (Low):** 1

### Issues by Category
- Design: 1 P2
- SOLID: 0
- Patterns: 1 P2
- Performance: 0
- Security: 1 P1
- Testing: 1 P2
- Documentation: 0

### Critical Issues (P0/P1)

#### Issue #1: SQL Injection Risk in Email Lookup
- **Severity:** P1 (High)
- **Category:** Security
- **File:** src/repositories/user_repository.py, line 42
- **Problem:** Email field not parameterized in SQL query
- **Why:** Attacker can inject SQL to access unauthorized data
- **Fix:** Use parameterized query with bound parameters
- **Estimated effort:** 15 minutes

### Medium Issues (P2)

#### Issue #2: Tight Coupling in AuthService
- **Severity:** P2
- **Category:** Design
- **File:** src/services/auth_service.py, line 15
- **Problem:** Hard-coded dependency on EmailService (new EmailService())
- **Why:** Can't mock in tests; violates Dependency Inversion
- **Fix:** Inject EmailService via constructor parameter
- **Estimated effort:** 30 minutes

### Low Issues (P3)

#### Issue #3: Code Duplication in Validators
- **Severity:** P3
- **Category:** Patterns
- **File:** src/validators/email.py and src/validators/password.py
- **Problem:** Both use similar regex validation pattern
- **Why:** Maintenance burden if validation rules change
- **Fix:** Extract shared regex into BaseValidator class
- **Estimated effort:** 45 minutes
```

### Section 4: Test Coverage Analysis

```markdown
## Test Coverage Analysis

### Coverage Metrics
- Production lines added: 215
- Test lines added: 120
- Estimated coverage: 75%

### Missing Test Scenarios

#### Function: create_user()
- [✅] Happy path: Valid email + password
- [⚠️] MISSING: Duplicate email error
- [⚠️] MISSING: Email service failure + retry
- [✅] Edge case: Long password

### Coverage Gaps
| Gap | Severity | Effort |
|-----|----------|--------|
| Duplicate email error test | P1 | 1 hour |
| Email failure + retry test | P1 | 1.5 hours |
| Password edge cases | P2 | 30 min |

### Overall Test Coverage: **75%**
**Recommendation:** Improve to 90%+ before merge
```

### Section 5: Documentation Analysis

```markdown
## Documentation Analysis

### Documentation Quality
- Docstrings: 90% (9/10 functions documented)
- Inline comments: 85% (complex logic explained)
- Usage examples: 40% (should be >70%)
- README/architecture: 0% (not provided)

### Documentation Gaps
1. Missing: How to handle signup errors in frontend
2. Missing: InvalidEmailError exception docs
3. Missing: Email service failure recovery strategy

### Overall Documentation: **80%**
**Recommendation:** Add usage examples before merge
```

### Section 6: Final Recommendations

```markdown
## Recommendations & Action Items

### Before Merge (Blocking)
- [ ] Fix SQL injection vulnerability (P1 Security)
  - Effort: 15 min
  - Impact: HIGH — blocks all signups if exploited
  
- [ ] Add missing error case tests (P1 Testing)
  - Effort: 2 hours
  - Impact: HIGH — untested failure paths
  
- [ ] Add retry logic for email service (P1 Testing)
  - Effort: 1.5 hours
  - Impact: HIGH — silent failures if email service fails

### Nice to Have (Non-Blocking)
- [ ] Refactor AuthService to inject EmailService (P2 Design)
  - Effort: 30 min
  - Impact: MEDIUM — improves testability

- [ ] Extract shared validation logic (P3 Patterns)
  - Effort: 45 min
  - Impact: LOW — reduces duplication

### Suggested Review Process
1. Author fixes P0/P1 issues (2-3 hours)
2. Re-run review to verify
3. Merge once score reaches B+ (85%+)
```

---

## 10. FAQ

### Q: What if JIRA requirement is vague?
**A:** Translate to best understanding and note the vagueness in the report. Suggest clarification in recommendations section. Score may be lower due to ambiguity.

### Q: What if no tests exist?
**A:** Test coverage scored as critically low (0-20%). Flag as P0 gap. Code cannot be merged without tests.

### Q: Can teams customize scoring weights?
**A:** Yes. Default weights are: Requirement (40%), Code Quality (30%), Testing (20%), Docs (10%). Can be adjusted per team policy. Document in CLAUDE.md.

### Q: What if code is perfect but requirement is unclear?
**A:** Grade will reflect the ambiguity. A "B" with note: "Excellent code quality, but requirement validation unclear. Clarify with product team before merge."

### Q: How do we handle third-party code or generated code?
**A:** Review as normal, but add note: "Third-party dependency. Severity scores may differ." For generated code, focus on integration points and configuration correctness rather than code quality.

### Q: What's the typical review time?
**A:** 15-30 minutes for <500 lines, 30-60 minutes for large PRs. Includes: requirement analysis, code review, test assessment, report generation.

### Q: Should reviews block merge or just be advisory?
**A:** Default: P0 issues block merge, P1 may block depending on team policy, P2/P3 are advisory. Configure in CLAUDE.md.

### Q: How do we handle security reviews?
**A:** All code flagged as P0/P1 security issues block merge unconditionally. Consider a separate "Security Review" phase with expert checklist for sensitive code (auth, payment, data).

---

## 11. Tech-Agnostic Nature

This skill is **tech-agnostic** because:

1. **All patterns are universal** — SRP, SOLID, N+1 queries exist in every language
2. **Severity scale is language-independent** — P0-P3 applies to Java, Python, React, SQL equally
3. **Process is framework-agnostic** — Works for Spring Boot, FastAPI, React, any stack
4. **Examples use pseudocode** — Concepts shown, not language-specific implementation
5. **Issue categories are universal** — Design, Security, Performance matter everywhere

**Language-specific guidance:** When implementing this skill for a specific language, create a supplementary guide with language-specific examples (e.g., "Java Security Checklist") but keep the core process in this skill.

---

## 12. Integration with Code Review Agent v3

This skill provides the **reusable review logic** used by **agents/code_review_agent.md**.

**Agent Flow:**
```
Code Review Agent v3
  ↓
  Apply Code Review Skill v3
    1. Requirement Analysis (Phase 1)
    2. Requirement Validation (Phase 2)
    3. Code Quality Review (Phase 3)
    4. Test Coverage Analysis (Phase 4)
    5. Documentation Analysis (Phase 5)
    6. Scorecard Calculation (Phase 6)
  ↓
  Generate HTML Report
  Post MR Comment
  Done
```

---

## 13. References & Standards

- **Master Rules:** `instructions/master_instruction_set.md`
- **SOLID Principles:** Robert C. Martin's "Clean Code"
- **Design Patterns:** Gang of Four, "Design Patterns: Elements of Reusable Object-Oriented Software"
- **Security:** OWASP Top 10, CWE Top 25
- **Code Quality:** SonarQube standards, ESLint/Pylint conventions
- **Testing:** AAA pattern (Arrange-Act-Assert), TDD best practices

---

## 14. Success Criteria

- ✅ Skill provides complete 6-phase logic
- ✅ All phases have clear process steps
- ✅ Scoring is transparent (formulas visible)
- ✅ Issue format is standardized (JSON structure)
- ✅ Tech-agnostic (no language-specific code)
- ✅ Examples cover happy path, error cases, edge cases
- ✅ FAQ addresses common questions
- ✅ Severity scale (P0-P3) clearly defined with triage rules

---

## Version History

| Version | Date | Changes |
|---------|------|---------|
| 3.0 | 2026-05-25 | Initial: 6-phase requirement-driven review logic |