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)

Security vulnerabilities (injection, auth bypass, secrets)
Financial precision errors (float/double for money)
Data integrity risks (race conditions, lost updates)
Breaking changes (undocumented API changes)

🟡 High (Should Fix)

Test coverage (<80%)
DDD violations (anemic models, leaked domain logic)
SOLID violations (tight coupling, god classes)
Error handling gaps (unhandled exceptions)

🟢 Medium (Nice to Fix)

Naming conventions (unclear variable names)
Code duplication (DRY violations)
Performance issues (N+1 queries)

⚪ Low (Optional)

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:

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

# 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%}

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)

{Critical issue 1}
{Critical issue 2}

Should Do (Current PR)

{High priority issue 1}
{High priority issue 2}

Nice to Do (Future PR)

{Medium priority issue}
{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:

{Action item 1}
{Action item 2}
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

Common Rationalizations

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

常見藉口	反制說明
"我已從作者角度完整審查"	⛔ 不能只從程式碼作者視角審查——必須依序切換至少 3 個 Specialist Lens（Security、Performance、Future Maintainer）再完成審查
"測試覆蓋率 80%，這個模組邏輯簡單不需要 100%"	Financial / Auth / Security 核心路徑要求 100% 覆蓋——沒有例外，「簡單」不是降低標準的理由
"這只是小改動，不需要完整安全稽核"	改動大小與安全風險無關——任何接觸認證、授權、金融精度的程式碼均需完整安全步驟
"PR 已通過所有自動測試，審查可以快速完成"	自動測試無法捕捉所有安全弱點——必須手動執行 Specialist Lens Review，不得以 CI green 替代人工審查

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 項目清單