# Reviewing Code > Conducts systematic code review with quality checks, architectural verification, and actionable feedback. Use when reviewing pull requests, code changes, or ensuring code quality standards. Use when this capability is needed. - Skill: `tomevault-io/reviewing-code-6` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add tomevault-io/reviewing-code-6` - Raw SKILL.md: https://api.skillmd.com/api/skills/tomevault-io/reviewing-code-6/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: tomevault-io (https://skillmd.com/u/tomevault-io) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/tomevault-io/reviewing-code-6 --- # Code Review Framework ## Core Principles - **Plan alignment first** - Implementation must match stated requirements - **Technical rigor** - Verify claims, don't assume - **Actionable feedback** - Specific issues with clear fixes - **Prioritized findings** - Critical vs Important vs Minor - **Constructive approach** - Acknowledge strengths, guide improvements ## Review Phases ### Phase 1: Plan Alignment Analysis **Purpose**: Verify implementation matches original requirements. **Process**: 1. Read the planning document, specification, or requirements 2. Review the implementation (git diff, changed files, new code) 3. Compare actual vs planned functionality 4. Identify deviations (both good and bad) **Evaluate**: - All planned functionality implemented? - Any missing features or partial implementations? - Deviations from planned approach? - Are deviations justified improvements or problematic departures? **Output**: List of alignment issues with severity --- ### Phase 2: Code Quality Assessment **Purpose**: Ensure code meets quality standards. **Review Areas**: #### Error Handling - Proper try/catch blocks or error returns - Meaningful error messages - Graceful degradation - Edge case handling #### Type Safety - Proper type annotations (TypeScript/typed languages) - No `any` types without justification - Correct use of generics - Validated inputs/outputs #### Code Organization - Logical file structure - Clear separation of concerns - Appropriate module boundaries - Consistent naming conventions #### Maintainability - Code is readable and self-documenting - Complex logic has explanatory comments - No unnecessary complexity - DRY principle followed **Output**: Quality issues categorized by area --- ### Phase 3: Architecture and Design Review **Purpose**: Validate architectural soundness. **Evaluate**: #### SOLID Principles - **S**ingle Responsibility: Each module/function has one job - **O**pen/Closed: Extensible without modification - **L**iskov Substitution: Subtypes are substitutable - **I**nterface Segregation: No fat interfaces - **D**ependency Inversion: Depend on abstractions #### Design Patterns - Appropriate patterns used correctly - No anti-patterns present - Patterns not overused or misapplied #### Integration - Integrates cleanly with existing systems - API contracts respected - Dependencies properly managed - No tight coupling to implementation details #### Scalability & Extensibility - Can handle growth in data/users - Easy to add new features - Configuration externalized where appropriate - No hard-coded limits that will break **Output**: Architectural concerns with recommendations --- ### Phase 4: Testing Review **Purpose**: Ensure adequate test coverage, quality, and TDD compliance. #### TDD Compliance (FIRST CHECK) **Verify tests were written BEFORE implementation:** **Questions to ask:** 1. **Does each function/method have corresponding tests?** ``` For each public function in implementation files: - [ ] Test file exists - [ ] Test covers function behavior - [ ] Test name clearly describes what's tested ``` **If NO**: Flag as Important issue ``` IMPORTANT: Missing tests for [function_name] in [file]. All functions require tests. Add tests before merging. ``` 2. **Request TDD Compliance Certification** Ask implementer to provide certification from practicing-tdd skill: "Please provide TDD Compliance Certification for each function (format from practicing-tdd skill)." **Verify:** - [ ] Certification provided for all new functions - [ ] All "Watched fail" are YES (or justified NO) - [ ] All "Watched pass" are YES - [ ] Failure reasons are specific (not vague) **If certification missing or incomplete**: Flag as Important issue ``` IMPORTANT: TDD Compliance Certification required. Must provide certification with one entry per function (see practicing-tdd skill). Certification proves RED-GREEN-REFACTOR cycle was followed. ``` **If author cannot provide certification**: Flag as Important issue ``` IMPORTANT: Unable to verify TDD followed for [function_name]. Tests may have been written after implementation. Recommend: Verify tests actually fail when implementation removed. ``` 3. **Do tests fail when implementation removed?** **If suspicious that tests written after** (e.g., all tests pass immediately, no git history showing test-first): ```bash # Verify tests actually test implementation # Comment out implementation git stash # Temporarily remove implementation npm test # Tests should FAIL git stash pop # Restore implementation ``` **If tests still pass with no implementation**: Flag as Critical issue ``` CRITICAL: Tests for [function_name] pass without implementation. Tests are not actually testing the code. See avoiding-testing-anti-patterns skill (Anti-Pattern 1: Testing Mock Behavior). Rewrite tests to verify real behavior. ``` 4. **Does git history suggest test-first?** **Optional check** (not definitive, but helpful indicator): ```bash git log --oneline --all -- tests/[file].test.ts src/[file].ts ``` **Look for:** - Test commits before implementation commits - Separate commits for RED → GREEN → REFACTOR - Commit messages mentioning TDD phases **If commits show implementation before tests**: Flag as Important issue ``` IMPORTANT: Git history suggests tests written after implementation. Commits show [file].ts before [file].test.ts. Verify TDD was followed. If not, recommend rewriting with TDD. ``` 5. **Are there any files with implementation but no tests?** ```bash # Find source files find src/ -name "*.ts" -o -name "*.js" # For each source file, check if test exists # tests/[name].test.ts or tests/[name].spec.ts ``` **If source file has no corresponding test file**: Flag as Important issue ``` IMPORTANT: No tests found for src/[file].ts All implementation files require tests. Add comprehensive tests before merging. ``` **TDD Compliance Summary:** After checking above: ```markdown ### TDD Compliance: [PASS / NEEDS IMPROVEMENT / FAIL] - Functions with tests: X / Y (Z% coverage) - Author attestation to RED-GREEN-REFACTOR: [Yes / No / Partial] - Tests verified to fail without implementation: [Yes / No / Not checked] - Files without tests: [List or "None"] [If NEEDS IMPROVEMENT or FAIL]: Recommendations: - Add tests for [list functions] - Verify tests for [list functions] actually fail when implementation removed - Consider rewriting [list functions] with TDD for higher confidence ``` #### Test Coverage - All new code paths tested - Edge cases covered - Error paths tested - Integration points verified #### Test Quality - Tests actually verify behavior (not just mock everything) - Tests are isolated and independent - Tests have clear arrange/act/assert structure - Test names describe what they verify #### Test Patterns and RED-GREEN-REFACTOR - Appropriate use of mocks/stubs/fakes - No test-only methods in production code - Tests fail when they should (RED-GREEN-REFACTOR) **See Also**: `avoiding-testing-anti-patterns` skill for what NOT to do, `practicing-tdd` skill for TDD process **Output**: Testing gaps, quality issues, and TDD compliance status --- ### Phase 5: Security & Performance **Purpose**: Identify vulnerabilities and performance issues. **Security Review**: - No SQL injection vulnerabilities - No XSS vulnerabilities - No command injection - Proper authentication/authorization - Sensitive data properly handled - No secrets in code **Performance Review**: - No obvious N+1 queries - Appropriate use of caching - No unbounded loops or recursion - Database queries optimized - Async operations used appropriately **Output**: Security and performance concerns --- ### Phase 6: Documentation Review **Purpose**: Verify code is properly documented. **Check**: - Public APIs documented - Complex algorithms explained - Non-obvious decisions justified in comments - README updated if needed - Breaking changes documented **Output**: Documentation gaps --- ## Issue Categorization Categorize all findings by **Priority**: ### Critical (Must Fix Before Merge) - Security vulnerabilities - Data corruption risks - Breaking existing functionality - Missing core requirements - Test failures ### Important (Should Fix Before Merge) - Missing error handling - Poor architecture that will cause problems - Significant performance issues - Missing important tests - Plan deviations without justification ### Minor (Nice to Have) - Style inconsistencies - Minor optimizations - Better naming suggestions - Additional test coverage - Documentation improvements --- ## Review Output Format Structure your review as follows: ```markdown # Code Review: [What Was Implemented] ## Summary **Overall Assessment**: [Ready to merge / Needs fixes / Major revision needed] **Strengths**: - [What was done well] - [Good decisions made] - [Quality aspects worth noting] **Issues Found**: [X] Critical, [Y] Important, [Z] Minor --- ## Critical Issues ### 1. [Issue Title] **Location**: `file/path.ts:123-145` **Issue**: [Specific problem with evidence] **Impact**: [Why this is critical] **Fix**: [Specific steps to resolve] --- ## Important Issues ### 1. [Issue Title] **Location**: `file/path.ts:67` **Issue**: [Specific problem] **Why Important**: [Consequences if not fixed] **Recommendation**: [How to fix] --- ## Minor Issues ### 1. [Issue Title] **Location**: `file/path.ts:89` **Suggestion**: [Improvement idea] **Benefit**: [Why this would help] --- ## Plan Alignment **Planned**: [What the plan said to do] **Implemented**: [What was actually done] **Deviations**: - [Deviation 1]: [Justified/Problematic and why] - [Deviation 2]: [Assessment] --- ## Testing Review ### TDD Compliance: [PASS / NEEDS IMPROVEMENT / FAIL] **Findings:** - Functions with tests: X / Y (Z% coverage) - Author attestation to TDD: [Yes / No / Partial] - Tests verified to fail without implementation: [Yes / No / Not checked] - Files without tests: [List or "None"] **Issues identified:** [List TDD compliance issues here] ### Test Coverage **Coverage**: [Percentage or qualitative assessment] **Gaps**: - [Untested scenario 1] - [Untested scenario 2] **Test Quality**: [Assessment] --- ## Recommendations **Immediate Actions** (before merge): 1. [Action 1] 2. [Action 2] **Future Improvements** (can defer): 1. [Improvement 1] 2. [Improvement 2] --- ## Approval Status - [ ] Critical issues resolved - [ ] Important issues addressed or acknowledged - [ ] Tests passing - [ ] Documentation updated **Status**: [Approved / Approved with minor items / Needs revision] ``` --- ## Integration with Workflows ### Requesting Review See `requesting-reviewing-code` skill for how to invoke this review process. **When to review**: - After completing each task (subagent-driven development) - After major features - Before merging to main - When stuck (fresh perspective) ### Receiving Review See `receiving-reviewing-code` skill for how to handle review feedback. **Key principles**: - Verify before implementing - Ask for clarification if unclear - Push back with technical reasoning if reviewer is wrong - No performative agreement ### Integration with Other Skills ## Workflow Checklist Copy this checklist to track your progress: See `assets/workflow-checklist.md` for the complete checklist. ## References For detailed information, see: - `references/detailed-guide.md` - Complete workflow details, examples, and troubleshooting --- > Converted and distributed by [TomeVault](https://tomevault.io/claim/bacchus-labs) — claim your Tome and manage your conversions.