SDLC Review Skill

This skill provides guidance for conducting comprehensive code reviews during the SDLC review phase.

Overview

Review is the quality gate before code can be merged. A thorough review evaluates code from multiple perspectives: code quality, security, and requirements fulfillment.

Review Methodology

Three-Perspective Review

Evaluate every implementation from these three perspectives:

1. Code Quality (Senior Developer Perspective)

Evaluate:

• •Code quality and maintainability
• •Following best practices and design patterns
• •Potential bugs or logic errors
• •Test coverage adequacy and test quality
• •Error handling completeness
• •Performance considerations

2. Security (Security Engineer Perspective)

Evaluate:

• •OWASP Top 10 vulnerabilities
• •Input validation and sanitization
• •Authentication and authorization issues
• •Data exposure risks
• •Command/SQL/code injection vulnerabilities
• •Secure coding practices

3. Requirements (Product Owner Perspective)

Evaluate:

• •Does it meet the acceptance criteria stated in the story?
• •Is the user experience appropriate and intuitive?
• •Are edge cases and error scenarios handled?
• •Is documentation adequate for users and maintainers?
• •Does the implementation align with the story goals?

Issue Severity Levels

Blocker 🛑

Must be fixed before merging. Examples:

• •Security vulnerabilities (injection, auth bypass)
• •Data corruption risks
• •Broken core functionality
• •Missing critical acceptance criteria

Critical ⚠️

Should be fixed before merging. Examples:

• •Major bugs affecting significant use cases
• •Poor practices with security implications
• •Missing error handling for common failures
• •Significant performance regressions

Major 📋

Should be addressed soon. Examples:

• •Code quality issues affecting maintainability
• •Missing edge case handling
• •Incomplete test coverage
• •Minor security concerns

Minor ℹ️

Nice to have improvements. Examples:

• •Style inconsistencies
• •Minor optimizations
• •Additional documentation
• •Code organization suggestions

Issue Categories

When categorizing issues, use these standard categories:

Review Output Format

Structure review findings as:

{
  "passed": true/false,
  "issues": [
    {
      "severity": "blocker" | "critical" | "major" | "minor",
      "category": "security" | "code_quality" | "testing" | etc,
      "description": "Detailed description of the issue",
      "file": "path/to/file.ts",
      "line": 42,
      "suggestedFix": "How to fix this issue",
      "perspectives": ["code", "security", "po"]
    }
  ]
}

Pass/Fail Decision Matrix

TDD Validation (When Enabled)

If TDD mode was enabled for the story, validate that:

1. •RED phase completed - Failing tests were written first
2. •GREEN phase completed - Minimal code made tests pass
3. •REFACTOR phase completed - Code was improved while keeping tests green
4. •No regressions - All tests still pass after each cycle

TDD violations are categorized as critical issues.

Deduplication Guidelines

Critical: Do NOT report the same underlying issue multiple times from different perspectives.

Bad Example (Duplicate Issues)

Issue 1 (code): "No tests exist for login function"
Issue 2 (security): "Login function lacks test coverage for security scenarios"
Issue 3 (po): "Acceptance criteria not verifiable without tests"

Good Example (Deduplicated)

Issue 1: "No tests exist for login function"
  - perspectives: ["code", "security", "po"]
  - suggestedFix: "Add unit tests covering happy path, invalid credentials, and rate limiting"

Security Review Checklist

Input Validation

• • All user inputs are validated and sanitized
• • File paths are validated (no path traversal)
• • SQL queries use parameterized statements
• • Shell commands use safe APIs (spawn, not exec with user input)

Authentication & Authorization

• • Auth checks on all protected routes
• • Session management is secure
• • Passwords are properly hashed (not stored plaintext)
• • Tokens have appropriate expiration

Data Protection

• • Sensitive data is not logged
• • Error messages don't leak internal details
• • PII is handled according to policy
• • Secrets are not hardcoded

OWASP Top 10 Quick Check

• • No injection vulnerabilities (SQL, command, LDAP)
• • Proper authentication mechanisms
• • Sensitive data encrypted in transit and at rest
• • XML/JSON parsers configured securely
• • Access control properly implemented

Code Quality Checklist

Readability

• • Functions have clear, single responsibilities
• • Variable names are descriptive
• • Complex logic is commented
• • No magic numbers (use named constants)

Maintainability

• • Follows existing codebase patterns
• • DRY principle applied (no significant duplication)
• • Dependencies are properly managed
• • Configuration is externalized appropriately

Error Handling

• • Errors are caught and handled appropriately
• • Error messages are helpful and actionable
• • Failures don't expose sensitive information
• • Recovery paths exist where appropriate

Testing

• • Unit tests exist for new functionality
• • Edge cases are tested
• • Tests are deterministic (no flaky tests)
• • Test code follows same quality standards as production

Requirements Review Checklist

Acceptance Criteria

• • Each AC has been addressed
• • Behavior matches the "Given-When-Then" scenarios
• • Edge cases mentioned in AC are handled
• • Default behaviors are sensible

User Experience

• • Error messages are user-friendly
• • Feedback is provided for long operations
• • Behavior is intuitive and predictable
• • Documentation is updated if needed

Completeness

• • No "TODO" or "FIXME" comments for required features
• • All planned files were created/modified
• • No temporary/debug code left behind
• • Feature is complete, not partially implemented

Tips for Effective Reviews

1. •

   Be specific - "Line 42 has SQL injection risk" not "Security issues exist"

2. •

   Provide fixes - Don't just identify problems, suggest solutions

3. •

   Prioritize correctly - Security issues trump style issues

4. •

   Check the diff - Focus on changed code, not unrelated existing issues

5. •

   Verify tests pass - Build and tests must pass before detailed review

6. •

   Consider context - Is this a quick fix or major feature? Adjust depth accordingly

7. •

   Be constructive - Goal is better code, not criticism