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

# 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:

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

# 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:

# 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:

# 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:

// ✅ 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%}

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

// ❌ BAD
double totalPrice = orderItems.Sum(x => x.Price * x.Quantity);

// ✅ GOOD
decimal totalPrice = orderItems.Sum(x => x.Price * x.Quantity);

Issue: Missing Idempotency

// ❌ BAD
[HttpPost("transactions")]
public async Task<IActionResult> CreateTransaction([FromBody] TransactionDto dto)
{
    var transaction = await _service.CreateAsync(dto);
    return Ok(transaction);
}

// ✅ GOOD
[HttpPost("transactions")]
public async Task<IActionResult> CreateTransaction(
    [FromBody] TransactionDto dto,
    [FromHeader(Name = "Idempotency-Key")] string idempotencyKey)
{
    if (string.IsNullOrEmpty(idempotencyKey))
        return BadRequest("Idempotency-Key required");
    
    var transaction = await _service.CreateOrGetAsync(dto, idempotencyKey);
    return Ok(transaction);
}

Issue: Anemic Domain Model

// ❌ BAD (Anemic)
public class Order
{
    public decimal Total { get; set; }
    public OrderStatus Status { get; set; }
}

// Service does all the logic
public class OrderService
{
    public void CompleteOrder(Order order)
    {
        order.Status = OrderStatus.Completed;
        order.Total = CalculateTotal(order);
    }
}

// ✅ GOOD (Rich domain model)
public class Order
{
    public decimal Total { get; private set; }
    public OrderStatus Status { get; private set; }
    
    public void Complete()
    {
        if (Status == OrderStatus.Cancelled)
            throw new InvalidOperationException("Cannot complete cancelled order");
        
        Status = OrderStatus.Completed;
        Total = CalculateTotalInternal();
    }
    
    private decimal CalculateTotalInternal() { /* domain logic */ }
}

Next Step

After review completion:

If issues found:

Fix critical issues → Re-run tests → Request re-review

If approved:

CLI:

Input: "archive 這個 change package"
[System loads work-archiving skill]
→ Generate the requested pre-merge 99-archive.md; update other local logs only when explicitly requested

VS Code:

Input: /archive
Or: "finalize and archive"

Or use workflow orchestrator:

Input: "what's next?"
[System validates Review content/status, then recommends Closeout when the selected package contract requires it]

Troubleshooting

"Too many issues found, overwhelming"

Solution: Fix critical (🔴) first, then high (🟡). Medium (🟢) can be separate PR.

"How strict should I be?"

Solution:

• Critical & High: Block merge
• Medium: Accept with follow-up issue
• Low: Optional, nice to have

"Should I review everything?"

Solution: Focus on:

• Business logic changes (high risk)
• Security-critical code
• Financial calculations
• Skip: Auto-generated code, minor formatting

Related Documentation

• Implementation Planning Skill - Previous stage
• Work Archiving Skill - Next stage
• DDD Good Practices

💡 Tip: A good review finds issues before they reach production. Be thorough but pragmatic—perfection is the enemy of shipping.

Common Rationalizations

在程式碼審查過程中，AI 可能以下列藉口略過關鍵步驟：

Verification

在產出 07-review.md 前，逐項確認（Gate = 交付前閘門；Verification = 自我完成確認）：

• 已依序切換 Security、Performance、Future Maintainer 三個 Specialist Lens，各視角均有記錄輸出
• 所有 🔴 Critical 問題已列出且有具體修復方案（不得只說「有問題」）
• Financial Precision 檢查完成：所有金錢欄位確認無 float/double（rg "float\|double" <changed-files> 無命中）
• 測試覆蓋率已量測，核心路徑 100%、整體 ≥80%（或明確說明例外理由）
• Security Checklist 所有項目已逐條確認（無靜默略過）
• 07-review.md 已建立，Decision 已填寫（PASS / PASS_WITH_NOTES / BLOCKED 其中之一）
• 若 Decision 為 BLOCKED，已明確列出 Must Fix 項目清單