code-security-review

# Code & Security Review > 💡 **Recommended Agent**: `code-reviewer-agent` (Senior Code Quality Auditor) > - **CLI**: Input `/agent` and select `code-reviewer-agent` > - **VS Code**: Use `@workspace #code-reviewer-agent` in Chat > > **⚠️ CLI Note**: Use natural language like "review 我的 code". VS Code users can use `/code-review` shortcut. ## When to Use This Skill Use this skill when: - Implementation is complete and ready for review - Before creating pull request - After TDD implementation phase - Suspicious code or security concerns - 實作完成,準備提交 PR 前 - 需要檢查程式碼品質與安全性 ## Prerequisites **Required**: - Code changes committed or staged (`git status` shows modifications) - Implementation phase complete **Recommended**: - `04-plan.md` to verify all tasks completed - Tests passing and coverage ≥80% ## Review Role Contract New Change Packages record independent Review in canonical `07-review.md`. Historical `05-review.md` remains readable; it is not the canonical filename for a new package. Canonical and legacy files may coexist only when the legacy file is the documented pointer-only alias—two independent Review bodies are competing evidence and blocking. Every Review body contains observable `Summary`, `Findings`, `Verification Evidence`, and `Decision`. Decision is exactly `PASS`, `PASS_WITH_NOTES`, or `BLOCKED`. Any unresolved Critical/High finding or required deterministic test/build/lint/static/gate failure requires `BLOCKED`. `agentic-eval` is self-evaluation, not independent Review. It cannot replace this Review when the selected mode requires independence, and neither self-evaluation nor prose can override deterministic failure. ## Review Priorities (High → Low) ### 🔴 Critical (Must Fix) 1. **Security vulnerabilities** (injection, auth bypass, secrets) 2. **Financial precision errors** (float/double for money) 3. **Data integrity risks** (race conditions, lost updates) 4. **Breaking changes** (undocumented API changes) ### 🟡 High (Should Fix) 5. **Test coverage** (<80%) 6. **DDD violations** (anemic models, leaked domain logic) 7. **SOLID violations** (tight coupling, god classes) 8. **Error handling gaps** (unhandled exceptions) ### 🟢 Medium (Nice to Fix) 9. **Naming conventions** (unclear variable names) 10. **Code duplication** (DRY violations) 11. **Performance issues** (N+1 queries) ### ⚪ Low (Optional) 12. **Style inconsistencies** (formatting, minor refactoring) ## Step-by-Step Review Process ### Step 1: Get Code Changes ```bash # Check git status git status # View staged/unstaged changes git diff # Or compare branch git diff main...feature-branch ``` ### Step 2: Run Security Audit Check for **Critical Security Issues**: | Issue | How to Detect | Fix | |-------|--------------|-----| | **Secrets in code** | Search for API keys, passwords, tokens | Move to environment variables | | **SQL injection** | Raw SQL with string concatenation | Use parameterized queries | | **XSS vulnerabilities** | Unescaped user input in HTML | Sanitize inputs, escape outputs | | **Auth bypass** | Missing authorization checks | Add RBAC/ABAC checks | | **Insecure dependencies** | `npm audit` / `pip-audit` | Update vulnerable packages | **Financial Systems Specific**: - ✅ Money fields use `decimal` (NOT float/double) - ✅ Idempotency keys validated for transactions - ✅ Audit logging present for sensitive operations - ✅ Timezone handling (store UTC, display local) ### Step 3: Code Quality Audit **DDD Compliance**: - [ ] Entities have identity and behavior (not anemic) - [ ] Value objects are immutable - [ ] Domain logic in domain layer (not controllers/API) - [ ] Aggregates enforce invariants - [ ] Domain events for cross-aggregate communication **SOLID Principles**: - [ ] Single Responsibility: Each class/function one purpose - [ ] Open/Closed: Extendable without modification - [ ] Liskov Substitution: Subtypes are substitutable - [ ] Interface Segregation: Small, focused interfaces - [ ] Dependency Inversion: Depend on abstractions **Naming Conventions** (C#): - [ ] PascalCase for classes, methods, properties - [ ] camelCase for local variables, parameters - [ ] Interfaces prefixed with `I` - [ ] Test methods: `MethodName_Condition_ExpectedResult` ### Step 4: Test Coverage Check ```bash # Run tests with coverage npm test -- --coverage # Or dotnet test /p:CollectCoverage=true ``` **Coverage Requirements**: - **80% minimum** for all code - **100% required** for: - Financial calculations - Authentication/authorization logic - Security-critical code - Core business logic **Test Quality**: - [ ] Tests verify behavior (not implementation) - [ ] Edge cases covered - [ ] Error scenarios tested - [ ] Integration tests for critical paths ### Step 5: Generate Review Document For a new package, create `changes/<YYYY-MM-DD>-<slug>/07-review.md`: Use the canonical structured role below. Every field appears exactly once, every option list is replaced by one selected value, and every evidence field is substantive: ```markdown # 07 Review ## Summary - Reviewed scope: [scope reviewed] - Independent reviewer: [reviewer identity] ## Findings - Critical: None | Resolved — evidence | Unresolved — evidence - High: None | Resolved — evidence | Unresolved — evidence - Medium: None | finding and disposition - Low: None | finding and disposition ## Verification Evidence - Targeted tests: PASS — evidence | BLOCKED — evidence | N/A — reason - Required full/static/project gates: PASS — evidence | BLOCKED — evidence | N/A — reason - Unavailable or unverified checks: None | WARNING — non-blocking evidence | BLOCKED — required deterministic evidence ## Decision - Decision: PASS | PASS_WITH_NOTES | BLOCKED - Rationale: [evidence-based rationale] ``` `None` is the explicit zero-finding value. `WARNING` is recorded and supports `PASS_WITH_NOTES` without blocking. Unresolved Critical/High findings or `BLOCKED` deterministic evidence require Decision `BLOCKED`. `agentic-eval` cannot replace this independent Review or override deterministic evidence. The extended example below is optional detail guidance, not the canonical semantic-role shape; it cannot replace or rename the structured fields above. --- **Optional detailed-review appendix example**: ```markdown # Code Review: {Feature Name} **Date**: {YYYY-MM-DD} **Reviewer**: {Name or "AI Agent"} **Status**: 🔴 Needs Work / 🟡 Minor Issues / 🟢 Approved --- ## Summary {Brief overview of changes and overall assessment} **Files Changed**: {X files} **Lines Added**: {+Y} **Lines Removed**: {-Z} --- ## Findings ### Critical Issues 🔴 (Must Fix Before Merge) ### Issue 1: {Title} **Severity**: Critical **File**: `{path/to/file.ts}:{line}` **Problem**: {Description of the issue} **Risk**: {What could go wrong} **Fix**: {How to resolve} **Code**: ```typescript // ❌ BAD double price = 19.99; // Floating point for money ``` **Recommended**: ```typescript // ✅ GOOD decimal price = 19.99M; // Decimal for money ``` --- ### Issue 2: {Title} {Repeat structure} --- ### High Priority Issues 🟡 (Should Fix) ### Issue 3: {Title} **Severity**: High **File**: `{path/to/file.ts}:{line}` **Problem**: {Description} **Fix**: {Solution} --- ### Medium Priority Issues 🟢 (Nice to Fix) ### Issue 4: {Title} **Severity**: Medium **File**: `{path/to/file.ts}:{line}` **Problem**: {Description} **Fix**: {Solution} --- ## Verification Evidence ### Security Checklist - [ ] No secrets or credentials in code - [ ] SQL injection prevented (parameterized queries) - [ ] XSS prevented (input sanitization, output escaping) - [ ] Authorization checks present - [ ] Dependencies up to date (no known vulnerabilities) - [ ] Money fields use decimal (NOT float/double) - [ ] Idempotency implemented for transactions - [ ] Audit logging for sensitive operations --- ### Financial Precision Checklist - [ ] Money stored as `decimal` or integer minor units - [ ] Currency explicitly stored (ISO 4217 code) - [ ] Idempotency-Key supported for transactional endpoints - [ ] Timezone: UTC storage, local display - [ ] Audit trail: Who, What, When logged --- ### Code Quality Assessment ### DDD Compliance - [ ] Entities have behavior (not anemic models) - [ ] Value objects are immutable - [ ] Domain logic in domain layer - [ ] Aggregates enforce invariants ### SOLID Principles - [ ] Single Responsibility - [ ] Open/Closed - [ ] Liskov Substitution - [ ] Interface Segregation - [ ] Dependency Inversion ### Naming & Style - [ ] Clear, descriptive names - [ ] Consistent formatting - [ ] No magic numbers/strings - [ ] Appropriate comments (why, not what) --- ### Test Coverage **Overall Coverage**: {X%} | Module | Coverage | Status | |--------|----------|--------| | `lib/transactions.ts` | 95% | ✅ Pass | | `api/v1/transactions` | 82% | ✅ Pass | | `lib/notifications.ts` | 75% | ⚠️ Below 80% | **Missing Coverage**: - {File/function 1}: {Why not covered} - {File/function 2}: {Recommendation} --- ### Performance Concerns ### Issue 1: {N+1 Query Problem} **File**: `{path}:{line}` **Problem**: {Description} **Impact**: {Performance degradation} **Fix**: {Use join or eager loading} --- ### Breaking Changes ⚠️ **API Breaking Change Detected** **Endpoint**: `POST /api/v1/users` **Change**: Response schema adds `notificationPreferences` field **Impact**: External clients with strict schema validation may break **Recommendation**: - Version bump to `/api/v2/users` - Maintain v1 for 2 weeks (deprecation period) - Announce to API consumers --- ### Recommendations ### Must Do (Before Merge) 1. {Critical issue 1} 2. {Critical issue 2} ### Should Do (Current PR) 1. {High priority issue 1} 2. {High priority issue 2} ### Nice to Do (Future PR) 1. {Medium priority issue} 2. {Refactoring opportunity} --- ## Decision **Reviewer Decision**: {Choose one} - **BLOCKED**: Deterministic failure or unresolved Critical/High findings - **PASS_WITH_NOTES**: No blockers; warnings or non-blocking follow-up remain - **PASS**: No blocking findings; required verification evidence is complete **Next Steps**: 1. {Action item 1} 2. {Action item 2} 3. After fixes, run Review again; after PASS/PASS_WITH_NOTES, complete applicable pre-merge Closeout --- ## Related Artifacts - Spec: `03-spec.md` (if selected) - Plan: `04-plan.md` - Test Plan: `05-test-plan.md` (if exists) - Git branch: `feature/{branch-name}` ``` --- ## Review Checklist Template Use this checklist during review: ```markdown ## Code Review Checklist ### Security ✅ - [ ] No secrets/credentials in code - [ ] SQL injection prevented - [ ] XSS prevented (input sanitization) - [ ] Authorization checks present - [ ] Dependencies secure (npm audit / pip-audit) ### Financial Precision ✅ - [ ] Money uses decimal (NOT float/double) - [ ] Currency stored explicitly - [ ] Idempotency for transactions - [ ] Audit logging present - [ ] Timezone handling correct (UTC storage) ### Code Quality ✅ - [ ] DDD: Domain logic in domain layer - [ ] SOLID principles followed - [ ] Clear naming conventions - [ ] No code duplication (DRY) - [ ] Error handling complete ### Testing ✅ - [ ] Test coverage ≥80% - [ ] Edge cases tested - [ ] Integration tests for critical paths - [ ] Tests verify behavior (not implementation) ### Performance ✅ - [ ] No N+1 query problems - [ ] Database indexes appropriate - [ ] Caching where beneficial - [ ] No memory leaks ### Breaking Changes ✅ - [ ] API changes documented - [ ] Migration guide provided (if needed) - [ ] Deprecation warnings added - [ ] Versioning strategy followed ``` ## Common Issues & Fixes ### Issue: Float/Double for Money ```csharp // ❌ BAD double totalPrice = orderItems.Sum(x => x.Price * x.Quantity); // ✅ GOOD decimal totalPrice = orderItems.Sum(x => x.Price * x.Quantity); ``` ### Issue: Missing Idempotency ```csharp // ❌ BAD