--- name: code-review-standards description: Code review checklist, severity definitions, and document templates. Load when performing code reviews or defining review criteria. license: MIT metadata: author: groupzer0 version: "1.0" --- # Code Review Standards Systematic approach to code review. Use this skill when: - Performing code reviews - Defining review criteria for a project - Understanding severity levels for findings - Creating code review documents --- ## Review Focus Areas Use this checklist when reviewing implementation code: | Category | What to Review | |----------|----------------| | **Architecture Alignment** | Does implementation match Architect's design? Follows system-architecture.md patterns? | | **SOLID Principles** | SRP, OCP, LSP, ISP, DIP violations (load `engineering-standards` for detection patterns) | | **DRY/YAGNI/KISS** | Duplication, speculative generalization, over-complexity | | **TDD Compliance** | TDD Compliance table present in implementation doc? All rows show test-first? | | **Code Smells** | Long Method, Large Class, Feature Envy, etc. (see `engineering-standards`) | | **Documentation & Comments** | Appropriate inline comments explaining "why" (not "what"), function docstrings, module-level docs, complex logic explained | | **Naming & Clarity** | Self-documenting names, appropriate abstractions, readable code | | **Error Handling** | Defensive coding, graceful failures, appropriate exceptions | | **Security Quick Scan** | Obvious vulnerabilities (injection, exposed secrets, hardcoded creds) | | **Performance** | Obvious inefficiencies, N+1 patterns, memory leaks | | **Observability** | Appropriate logging, telemetry for debugging | --- ## Severity Levels | Severity | Definition | Action | |----------|------------|--------| | **CRITICAL** | Security vulnerability, data loss risk, architectural violation | REJECT - must fix | | **HIGH** | Anti-pattern, significant maintainability issue, missing tests | REJECT - must fix | | **MEDIUM** | Code smell, minor design issue, unclear code | Fix recommended, may approve with comments | | **LOW** | Style preference, minor optimization opportunity | Note for future, approve | | **INFO** | Observation, suggestion for improvement | FYI only | ### When to Reject - Any CRITICAL finding → REJECT - Any HIGH finding → REJECT - 3+ MEDIUM findings in same file → Consider REJECT - Pattern of MEDIUM findings across files → Consider REJECT --- ## Finding Format When documenting findings, use this format: ```markdown **[SEVERITY] [Category]**: [Brief title] - **Location**: `path/to/file.py:L42-L55` - **Issue**: [What's wrong and why it matters] - **Recommendation**: [Specific fix suggestion] ``` **Example:** ```markdown **[HIGH] Documentation**: Missing docstrings on public API - **Location**: `src/api/handlers.py:L15-L45` - **Issue**: Public functions `create_user()` and `delete_user()` lack docstrings. Future maintainers won't understand expected inputs/outputs. - **Recommendation**: Add Google-style docstrings with Args, Returns, and Raises sections. ``` --- ## Code Review Document Template Create in `agent-output/code-review/` matching plan name: ```markdown # Code Review: [Plan Name] **Plan Reference**: `agent-output/planning/[plan-name].md` **Implementation Reference**: `agent-output/implementation/[plan-name]-implementation.md` **Date**: [date] **Reviewer**: Code Reviewer ## Changelog | Date | Agent Handoff | Request | Summary | |------|---------------|---------|---------| | YYYY-MM-DD | [Who handed off] | [What was requested] | [Brief summary] | ## Architecture Alignment **System Architecture Reference**: `agent-output/architecture/system-architecture.md` **Alignment Status**: ALIGNED / MINOR_DEVIATIONS / MAJOR_DEVIATIONS [Assessment of how implementation aligns with architectural decisions] ## TDD Compliance Check **TDD Table Present**: Yes / No **All Rows Complete**: Yes / No **Concerns**: [Any issues with TDD compliance] ## Findings ### Critical [List of critical findings, or "None"] ### High [List of high findings, or "None"] ### Medium [List of medium findings, or "None"] ### Low/Info [List of low/info findings, or "None"] ## Positive Observations [Acknowledge good patterns, well-written code, or improvements] ## Verdict **Status**: APPROVED / APPROVED_WITH_COMMENTS / REJECTED **Rationale**: [Brief explanation] ## Required Actions [If rejected: specific list of fixes required] [If approved with comments: optional improvements] ## Next Steps [Handoff to Implementer for fixes / Handoff to QA for testing] ``` --- ## Review Best Practices 1. **Be constructive**: Explain WHY something is an issue, not just THAT it's an issue 2. **Be specific**: File paths, line numbers, code snippets 3. **Provide solutions**: Concrete fix suggestions, not just criticism 4. **Acknowledge good work**: Note positive patterns when you see them 5. **Prioritize**: Focus on impactful issues, not nitpicks 6. **Context matters**: Consider the plan's constraints and timeline