feat: 集成code-review skill到项目

- 项目级 skill: .claude/skills/code-review/ (398行SKILL.md + 参考文件)
- 自动触发: AI修改.py/.cbl/.cpy/.lark后自动review
- CLAUDE.md: 定义触发规则、review流程、严重级别
- .code-review.yaml: tier=standard, 高风险模块配置

效果: clone即用, 每次代码变更后自动审查, 防止低质量代码入库
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
NB-076
2026-06-25 10:24:15 +08:00
parent 874b16f48c
commit 2f61ad7f1a
16 changed files with 1146 additions and 0 deletions
+398
View File
@@ -0,0 +1,398 @@
---
name: code-review
description: Use when reviewing AI-generated backend code, or when user says "review", "审查", "验收", "audit this code", "检查代码".
---
# code-review
> All file paths in checklist references are relative to this skill's directory (`code-review/references/`).
## Overview
Three-layer progressive review that catches AI-generated code defects AI commonly misses — transaction consistency, idempotency, distributed locks, parameter validation, SQL performance, concurrency safety, sensitive data masking, permission control, and error fallback. Outputs a scored text report with auto-fix and manual review gates, plus an exported standalone HTML report (`code-review-report.html`).
## Invocation
Two paths into this skill:
- **Auto-trigger:** After AI generates or modifies any backend code (controllers, services, data access, utilities, middleware, config), automatically enter review.
- **Manual trigger:** User says "审查", "review", "验收", "audit this code", "检查代码", "code review".
## Skip & Risk Alert
User may skip review with "跳过审查" or "不用审查". Before skipping, check if the change touches high-risk areas: authentication, payment, permissions, database schema changes, or data migration. If yes, issue a brief risk alert:
> ⚠️ 检测到高风险模块变更({area}),建议执行审查后再合并。
If user still chooses to skip, respect it.
## When NOT to Use
- **Trivial changes** — single-line fixes, comment changes, formatting-only diffs
- **Frontend-only code** — this skill is scoped for backend code review
- **User explicitly declines** — user says "跳过审查", "no review needed", "skip review"
- **Throwaway prototypes** — experimental code that won't reach CI/CD or production
## Review Tier Configuration
Check for `.code-review.yaml` in the project root to determine the review tier. If absent, default to **standard**.
```yaml
# .code-review.yaml
tier: standard # fast | standard | strict
```
| Tier | Scope | Behavior |
|------|-------|----------|
| **fast** | Prototypes, non-critical modules | Layer 1 scan + 🔴 blocker check only. Skip 🟡 and 🔵. |
| **standard** (default) | Regular business code | All three layers. 🔴 + 🟡 issues must be fixed. |
| **strict** | Core: payment, permissions, data migration | Full review + mandatory manual review + 🔴🟡 fixes require per-item user confirmation. |
Single-invocation override: "快速审查" → fast, "严格审查" → strict.
## Review Flow
### Step 0: Determine Scope
Analyze the changes to determine review granularity:
- **Change-level** (default): Only review AI-generated/modified code blocks and affected adjacent logic.
- **File-level**: When change involves file reorganization.
- **Service-level**: When cross-service call changes exist, associate upstream/downstream.
- **Chain-level**: When the full call chain is affected (DB → DAO → Service → Controller).
Declare scope at the top of the review report.
---
### Layer 1: Chain Decomposition & Inspection
Decompose code into seven categories. For each, identify high-risk gaps that AI commonly misses:
1. **Interface Layer** — Parameter validation, response conventions, HTTP status codes, rate limiting
2. **Business Layer** — Business logic alignment with requirements, state machine correctness, idempotency design, distributed locks
3. **Data Layer** — SQL injection, query performance, index usage, transaction boundaries and consistency
4. **Utility Layer** — Input validity, no side effects, error return values
5. **Error Handling** — Exception classification, fallback logic, error message sanitization, retry strategies
6. **Security** — AuthN/AuthZ, sensitive data masking, permission control, CSRF/XSS prevention
7. **Performance** — N+1 queries, caching strategy, connection pooling, batch operations
Also verify: does not deviate from requirements; does not violate architecture conventions.
For each category, mark: ✅ Clean / ⚠️ Issues Found / — Not Applicable.
---
### Layer 2: Quantitative Quality Analysis
Evaluate against measurable metrics. No subjective assessment.
| Metric | How to Measure |
|--------|---------------|
| **Requirement Coverage** | Map requirement items → code implementation. Flag uncovered items. |
| **Logic Alignment** | Compare against requirement docs (or baseline if no docs). Accuracy of business rules. |
| **Exception Branch Coverage** | Count normal paths vs. exception paths. Flag gaps. |
| **SQL Performance Risk** | Detect slow queries, missing indexes, full table scans. |
| **Code Redundancy Rate** | Identify extractable common logic, copy-paste code. |
| **Vulnerability Risk Rate** | Check against OWASP / security standards. |
| **High-Risk Scenario Coverage** | Concurrency, transactions, data consistency, race conditions. |
**No-Requirements Baseline:** When no requirement document exists, use:
1. Industry standards (OWASP, RESTful conventions, SQL best practices)
2. Similar business best practices (by project type: ecommerce/social/SaaS)
3. Previously accumulated prompt knowledge
4. Existing architecture conventions in the project
Mark the report with "No requirements doc — reviewed against generic baseline" and reduce weights of requirement coverage and logic alignment.
**Output Classification:**
- **Ready** — Passes review, ready for use
- **Needs Fix** — Has issues but fixable, with fix plan attached
- **Unusable** — Cannot be used, needs rewrite, with reason stated
---
### Layer 3: Optimization & Acceptance Standards
#### Standards Enforcement
- **Code standards** — naming, structure, comments
- **Security standards** — input validation, output encoding, permission checks
- **Performance standards** — query optimization, caching, async processing
#### Tool Scanning (if applicable to the language/ecosystem)
- Code scanning (linter, static analysis) — run and report
- SQL check (EXPLAIN, slow queries, index suggestions) — run and report
- Vulnerability detection (dependency vulnerabilities, injection points) — run and report
#### Manual Review Checklist
High-risk modules are force-flagged for manual review. Load the specialized checklist from `references/manual-review/{module}.md` when relevant:
| Module | File |
|--------|------|
| Payment | `references/manual-review/payment.md` |
| Order | `references/manual-review/order.md` |
| Inventory | `references/manual-review/inventory.md` |
| Permission | `references/manual-review/permission.md` |
| Distributed Lock | `references/manual-review/distributed-lock.md` |
| Data Migration | `references/manual-review/data-migration.md` |
#### Knowledge Accumulation
After each review, identify patterns for prompt accumulation:
- **Business Rules** (same module repeating same logic errors) → suggest accumulation
- **Historical Pitfalls** (same error pattern ≥2 occurrences) → suggest accumulation
- **Architecture Constraints** (violations of project architecture conventions) → suggest accumulation
Propose accumulation items at the end of the report. User confirms before adding to prompt library.
**Accumulation workflow:**
```text
Review findings → auto-extract high-frequency/high-risk issues
→ ask user to confirm (avoid noise)
→ record to prompt library (categorized: business rules / pitfalls / architecture constraints)
→ next AI generation auto-loads matched prompts
→ compare issue rates over time to verify effectiveness
```
Unconfirmed accumulations do not auto-activate.
#### Acceptance Gate
```text
AI draft → Skill review (auto) → Manual review & fix → Code review → Deploy
AI raw code must NOT go directly to production
```
---
### Step 4: Export HTML Report
After the review is complete and all fixes are applied, generate a standalone HTML report. Follow the instructions in the [HTML Report Export](#html-report-export) section below.
---
## Severity Levels
| Level | Meaning | Examples |
|-------|---------|----------|
| 🔴 Blocker | Must fix immediately. Cannot merge/deploy. | SQL injection, hardcoded secrets, missing transaction causing data inconsistency, auth bypass |
| 🟡 Major | Should fix before merge. | Missing input validation, missing error fallback, N+1 query, unmasked sensitive data |
| 🔵 Minor | Can optimize later. | Non-standard naming, duplicate code, missing comments, optimizable loop |
**Rule:** If any 🔴 blocker exists → verdict is ❌ FAIL regardless of other scores.
---
## Auto-Fix Strategy
Fix order: 🔴 Blocker → 🟡 Major → 🔵 Minor
### Fix Boundary Rules
| Issue Type | Auto-Fix | Manual Only |
|-----------|----------|-------------|
| Missing parameter validation | ✅ Add validation | — |
| SQL injection | ✅ Parameterized queries | — |
| N+1 queries | ✅ Batch queries | — |
| Sensitive data masking | ✅ Apply masking | — |
| Transaction boundaries | ⚠️ Single-table/single-DB transactions | Cross-DB/nested/distributed |
| Idempotency design | — | ❌ Business confirmation needed |
| Distributed locks | — | ❌ Architecture confirmation needed |
| Permission model | — | ❌ Product confirmation needed |
### Post-Fix Regression
After each fix:
1. **Regression scope:** Re-run Layer 1 scan on all modified files
2. **Re-check metrics:** Re-run affected Layer 2 metrics
3. **Rollback condition:** If fix introduces new 🔴 or 🟡 issue → auto-rollback, mark as "Fix Failed", list under manual review
4. **Fix report:** Each fix marked "Fix Success / Fix Failed / Rolled Back"
5. **Re-export HTML:** Regenerate `code-review-report.html` after all fixes in the batch are applied
### Before Fixing
Always present the fix plan to the user and wait for confirmation before executing.
---
## HTML Report Export
After all review layers and fixes are complete, generate a standalone HTML report file.
### HTML Generation
1. Load the HTML template from `references/report-template.html`
2. Populate the template placeholders (`{{PLACEHOLDER}}`) with actual review data
3. Write the output as `code-review-report.html` in the project root directory
### Template Placeholders
| Placeholder | Description |
|-------------|-------------|
| `{{SCOPE}}` | Review scope level |
| `{{TIER}}` | Review tier |
| `{{TIMESTAMP}}` | Current timestamp |
| `{{FILES}}` | Comma-separated file list |
| `{{BASELINE}}` | Review baseline (requirements doc / generic) |
| `{{VERDICT}}` | `✅ PASS` or `❌ FAIL` |
| `{{VERDICT_CLASS}}` | CSS class: `pass` or `fail` |
| `{{L1_*}}` | Layer 1 status: `✅ Clean` / `⚠️ Issues Found` / `— N/A` |
| `{{L1_*_CLASS}}` | CSS class: `status-clean` / `status-issues` / `status-na` |
| `{{REQ_COVERAGE}}` | Requirement coverage percentage |
| `{{REQ_COV_CLASS}}` | CSS class: `metric-good` / `metric-warn` |
| `{{LOGIC_ALIGNMENT}}` | Logic alignment percentage |
| `{{EXC_COVERAGE}}` | Exception coverage percentage |
| `{{EXC_COV_CLASS}}` | CSS class |
| `{{SQL_RISK}}` | SQL risk level |
| `{{SQL_RISK_CLASS}}` | CSS class |
| `{{REDUNDANCY_RATE}}` | Redundancy rate |
| `{{VULN_RISK}}` | Vulnerability rate |
| `{{VULN_RISK_CLASS}}` | CSS class |
| `{{HRISK_COVERAGE}}` | High-risk scenario coverage |
| `{{HRISK_CLASS}}` | CSS class |
| `{{READY_COUNT}}` / `{{NEEDS_FIX_COUNT}}` / `{{UNUSABLE_COUNT}}` | Classification counts |
| `{{BLOCKER_COUNT}}` / `{{MAJOR_COUNT}}` / `{{MINOR_COUNT}}` | Severity counts |
| `{{ISSUES_LIST}}` | HTML markup for all issues, each as a `.issue` div |
| `{{MANUAL_REVIEW_SECTION}}` | HTML markup for manual review items, or empty string |
| `{{ACCUMULATION_SECTION}}` | HTML markup for knowledge accumulation suggestions, or empty string |
### Issue HTML Format
Each issue in `{{ISSUES_LIST}}` should follow this structure:
```html
<div class="issue issue-blocker">
<span class="tag tag-blocker">🔴</span>
<span class="category">[Data]</span>
Missing transaction
<div class="location">→ order-service.ts:156</div>
<div class="fix">Fix: Added transaction wrap</div>
</div>
```
Use `issue-blocker` / `issue-major` / `issue-minor` and `tag-blocker` / `tag-major` / `tag-minor` accordingly.
### Manual Review HTML Format
```html
<div class="section">
<h2>Manual Review Required</h2>
<div class="manual-item">Payment callback idempotency</div>
<div class="manual-item">Distributed lock timeout</div>
</div>
```
### Knowledge Accumulation HTML Format
```html
<div class="section">
<h2>Knowledge Accumulation Suggestions</h2>
<div class="accum-item">Business Rules — order status state machine errors</div>
<div class="accum-item">Historical Pitfalls — missing transaction wrap in batch operations</div>
</div>
```
### File Output
The generated HTML file is saved as `code-review-report.html` in the project root directory. It is a fully self-contained file with all styles inlined — no external dependencies.
---
## Report Format (Text)
```text
╔══════════════════════════════════╗
║ Backend Code Review Report ║
╠══════════════════════════════════╣
║ Scope: {change-level/file-level/service-level/chain-level} ║
║ Files: {file list} ║
║ Time: {timestamp} ║
║ Tier: {fast/standard/strict} ║
║ Verdict: ✅ PASS / ❌ FAIL ║
╠══════════════════════════════════╣
║ Layer 1 · Chain Decomposition ║
║ Interface: ✅ Business: ⚠️ ║
║ Data: ✅ Utility: ✅ ║
║ Error: ⚠️ Security: ✅ ║
║ Performance: ✅ ║
╠══════════════════════════════════╣
║ Layer 2 · Quantitative Metrics ║
║ Requirement Coverage: 85% (3/5 uncovered) ║
║ Logic Alignment: 90% ║
║ Exception Coverage: 60% ⚠️ ║
║ SQL Risk: Low ║
║ Redundancy Rate: 12% ║
║ Vulnerability Rate: 0% ║
║ High-Risk Coverage: Missing concurrency ║
╠══════════════════════════════════╣
║ Classification ║
║ Ready: 3 Needs Fix: 2 Unusable: 0 ║
╠══════════════════════════════════╣
║ 🔴 Blocker: 1 🟡 Major: 3 🔵 Minor: 4 ║
╠══════════════════════════════════╣
║ Issues: ║
║ 🔴 [Data] Missing transaction ║
║ → order-service.ts:156 ║
║ → Fix: Added transaction wrap ║
║ 🟡 [Security] Unvalidated input ║
║ → auth.ts:42 ║
║ → Fix: Added validation ║
║ ... ║
║ Manual Review Required: ║
║ → Payment callback idempotency ║
║ → Distributed lock timeout ║
╚══════════════════════════════════╝
```
### Report Interaction
- **Issue navigation:** Issue line numbers link to code location
- **Fix preview:** Fix plan shows diff preview
- **Batch operations:** Confirm/deny fixes by severity level; batch-skip 🔵 items
- **Per-item confirmation:** 🔴 and high-risk 🟡 fixes confirmed one at a time
---
## Language Adaptation
Detect project language/framework and load the corresponding checklist from `references/`:
| Language/Framework | Checklist File |
|-------------------|---------------|
| Java + Spring Boot | `references/java-spring.md` |
| Python + Django | `references/python-django.md` |
| Python + FastAPI | `references/python-fastapi.md` |
| Node.js + Express | `references/node-express.md` |
| Go + Gin | `references/go-gin.md` |
| No match | Use `references/review-checklist.md` (generic) |
---
## Constraints
- **No silent modifications:** Present fix plan before executing. Get user confirmation.
- **No fixes on FAIL:** When 🔴 blockers cause ❌ FAIL, report first and wait for user decision.
- **Context-aware:** With requirement docs → full review. Without → use "no-requirements baseline", reduce requirement/logic metric weights.
- **Historical learning:** High-frequency/high-risk findings auto-suggested for prompt accumulation. User confirms before activation.
- **Language-adaptive:** Auto-detect project language/framework, load matching checklist. Fall back to generic.
- **Config-driven:** `.code-review.yaml` controls review tier. No hardcoded rules.
---
## Common Mistakes
| Mistake | Fix |
|---------|-----|
| Rushing review without checking each layer | Complete all three layers in order. Layer 1 catches structural issues, Layer 2 catches metrics gaps, Layer 3 catches process gaps. |
| Skipping no-requirements baseline and guessing requirements | When no req docs exist, explicitly use the 4-part baseline (industry standards, business practices, accumulated prompts, architecture conventions). Mark the report accordingly. |
| Fixing 🔵 minor issues before 🔴 blockers | Fix order is strict: 🔴 → 🟡 → 🔵. A 🔴 blocker means ❌ FAIL — fix it first or don't fix at all. |
| Applying fixes without user confirmation | Always present the fix plan and wait for confirmation before executing. |
| Forgetting to mark high-risk modules for manual review | Payment, order, inventory, permissions, distributed locks, and data migration always require manual review. Load the corresponding checklist. |
| Loading the wrong language checklist | Auto-detect language/framework from project config (package.json, pom.xml, go.mod, etc.). Fall back to generic checklist if detection fails. |
| Omitting the "No requirements doc" annotation | Always state the review baseline in the report header so readers know what standards were applied. |
| Forgetting to export the HTML report after fixes | After all fixes are applied, always generate the HTML report (`code-review-report.html`) so the final output reflects the latest state. |
@@ -0,0 +1,48 @@
# Go Gin Review Checklist
Extends the generic checklist with Go/Gin-specific items.
## Interface Layer (Gin Handlers)
- [ ] `ShouldBindJSON` / `ShouldBindQuery` with error handling
- [ ] Binding structs have `binding:"required"` tags
- [ ] Custom validators registered with `binding.Validator`
- [ ] Response helpers used consistently (not raw `c.JSON` everywhere)
- [ ] Middleware applied at appropriate scope (global vs group vs handler)
## Business Layer
- [ ] Business logic in service structs with interfaces
- [ ] Context propagation (`context.Context`) through all layers
- [ ] Dependency injection via constructor, not global variables
## Data Layer (GORM / sqlx / database/sql)
- [ ] GORM: `Preload()` instead of lazy loading in loops
- [ ] GORM: `Where("field = ?", value)` — parameterized queries
- [ ] database/sql: prepared statements with placeholders
- [ ] Connection pool: `SetMaxOpenConns`, `SetMaxIdleConns`, `SetConnMaxLifetime`
- [ ] `rows.Close()` always called (or use `defer`)
## Error Handling
- [ ] Errors wrapped with `fmt.Errorf("context: %w", err)` for traceability
- [ ] `errors.Is()` and `errors.As()` for error type checking
- [ ] No `panic()` in request handlers (use recovery middleware)
- [ ] Gin recovery middleware configured
## Security
- [ ] `gin-contrib/cors` with specific origins
- [ ] Rate limiting middleware (e.g., `gin-contrib/limiter`)
- [ ] JWT or session middleware for auth
- [ ] Secrets from environment, never committed
- [ ] `gin.SetMode(gin.ReleaseMode)` in production
## Performance
- [ ] Goroutine pools for concurrent operations (avoid unbounded goroutines)
- [ ] `sync.Pool` for frequently allocated objects
- [ ] Database query limits on all SELECTs
- [ ] `context.WithTimeout` for all external calls
- [ ] JSON serialization with `json:"-"` on sensitive fields
@@ -0,0 +1,48 @@
# Java Spring Boot Review Checklist
Extends the generic checklist with Java/Spring-specific items.
## Interface Layer (Spring MVC)
- [ ] `@RestController` methods have `@Valid` on request bodies
- [ ] Custom validators implement `ConstraintValidator` correctly
- [ ] `@ExceptionHandler` or `@ControllerAdvice` for global error handling
- [ ] `@ResponseStatus` used appropriately on custom exceptions
- [ ] DTOs use records or Lombok `@Data` — not exposing entities directly
- [ ] `@RequestMapping` produces/consumes specified
## Business Layer (Spring Service)
- [ ] `@Transactional` on service methods that modify multiple tables
- [ ] `@Transactional(rollbackFor = Exception.class)` — not just RuntimeException
- [ ] Transaction propagation set correctly (REQUIRED vs REQUIRES_NEW)
- [ ] No `@Transactional` on private methods (proxy limitation)
## Data Layer (Spring Data JPA / MyBatis)
- [ ] JPA: `@Entity` classes have proper `equals()` and `hashCode()`
- [ ] JPA: No eager fetching on `@ManyToOne` without explicit need
- [ ] JPA: `@Query` with nativeQuery=false by default (prevent SQL injection)
- [ ] MyBatis: All SQL uses `#{}` not `${}` for user input
- [ ] Connection pool settings reviewed (HikariCP defaults usually fine)
## Error Handling
- [ ] Checked exceptions either handled or declared
- [ ] `try-with-resources` for AutoCloseable resources
- [ ] No `catch (Exception e) { e.printStackTrace(); }` — use logger instead
## Security (Spring Security)
- [ ] `SecurityFilterChain` configured correctly
- [ ] CSRF protection enabled for state-changing endpoints
- [ ] `@PreAuthorize` or `@Secured` on protected methods
- [ ] Password encoding uses `BCryptPasswordEncoder` or better
- [ ] No secrets in `application.properties` — use env vars or vault
## Performance
- [ ] `@Async` used for non-blocking operations with proper thread pool config
- [ ] `@Cacheable` with TTL and eviction strategy
- [ ] JPA: `@BatchSize` or batch insert for bulk operations
- [ ] RestTemplate/WebClient timeouts configured
@@ -0,0 +1,25 @@
# Data Migration — Manual Review Checklist
## Rollback Plan
- [ ] Rollback script tested and ready before migration
- [ ] Rollback handles partial migration state
- [ ] Rollback is idempotent
- [ ] Database backup taken before migration
## Consistency Verification
- [ ] Migration verification query defined (row count, checksum, sample compare)
- [ ] Old and new data validated before old table dropped
- [ ] Foreign key constraints preserved
- [ ] Indexes migrated or recreated
## Large Table Strategy
- [ ] Batch processing with configurable batch size
- [ ] Migration doesn't lock table for extended period
- [ ] Progress tracking (how many rows processed)
- [ ] Rate limiting to avoid DB overload
- [ ] Dry-run mode tested before actual migration
## Idempotency
- [ ] Migration script is idempotent (can be re-run safely)
- [ ] Uses INSERT ON CONFLICT / INSERT IGNORE / MERGE as appropriate
- [ ] Migration tracks what has been processed (checkpoint table)
@@ -0,0 +1,23 @@
# Distributed Lock — Manual Review Checklist
## Timeout Strategy
- [ ] Lock TTL defined and justified (not arbitrary)
- [ ] TTL exceeds maximum expected operation time
- [ ] Watchdog/refresh mechanism if operation may exceed TTL
- [ ] Lock auto-releases on process crash (not permanent deadlock)
## Release Mechanism
- [ ] Lock released by the same instance that acquired it (ownership check)
- [ ] Release is atomic (check-then-release is not atomic — use Lua script or compare-and-delete)
- [ ] Lock release in finally/defer block guaranteed to execute
## Retry Strategy
- [ ] Retry with exponential backoff, not infinite retry
- [ ] Maximum retry count or timeout defined
- [ ] Failed lock acquisition returns clear error (not silent fallback)
## Granularity & Deadlock
- [ ] Lock key granularity appropriate (per-resource, not global)
- [ ] Multiple lock acquisition order consistent (prevents deadlock)
- [ ] No nested locks on same resource
- [ ] Lock contention monitored/metrics available
@@ -0,0 +1,21 @@
# Inventory Module — Manual Review Checklist
## Concurrency Safety
- [ ] Stock deduction uses atomic operations (optimistic lock, Redis decr, DB atomic update)
- [ ] No SELECT + UPDATE gap for stock operations
- [ ] Lock granularity: per-SKU, not global lock on inventory table
## Pre-sale / Reservation
- [ ] Pre-sale inventory separated from actual inventory
- [ ] Reservation timeout and auto-release mechanism
- [ ] Reservation-to-actual-deduction transition is atomic
## Audit Trail
- [ ] Every stock change logged with: SKU, quantity, operation type, order ID, timestamp
- [ ] Stock reconciliation queryable from audit log
- [ ] No direct DB updates bypassing the service layer
## Stock Recovery
- [ ] Order cancellation triggers stock recovery
- [ ] Recovery is idempotent (double-cancel doesn't double-recover)
- [ ] Recovery handles edge case: stock recovered after SKU deleted
@@ -0,0 +1,23 @@
# Order Module — Manual Review Checklist
## State Machine
- [ ] All order states defined and documented
- [ ] Valid transitions enforced (no impossible state changes)
- [ ] Terminal states properly handled (no further mutations)
- [ ] State change audit log exists
## Concurrent Deduction
- [ ] Inventory deduction uses optimistic lock (version number) or pessimistic lock (SELECT FOR UPDATE)
- [ ] No race condition between check and deduction (not SELECT → check → UPDATE)
- [ ] Oversell prevention verified
## Timeout Cancellation
- [ ] Unpaid order timeout mechanism (scheduled job, delayed queue, or TTL)
- [ ] Cancellation releases held inventory
- [ ] Cancellation releases held coupons/promotions
- [ ] Timeout is configurable, not hardcoded
## Distributed Transaction
- [ ] Cross-service operations (order + inventory + payment) have compensation logic
- [ ] Saga pattern or eventual consistency defined
- [ ] Failure recovery documented (what happens if order succeeds but payment fails)
@@ -0,0 +1,26 @@
# Payment Module — Manual Review Checklist
## Callback Idempotency
- [ ] Is payment callback idempotent? (duplicate notification won't double-charge)
- [ ] Idempotency key sourced from payment provider's transaction ID
- [ ] Idempotency check happens before any state change
## Amount Precision
- [ ] All monetary amounts use integer cents or decimal with fixed precision
- [ ] No floating-point arithmetic in payment calculations
- [ ] Rounding strategy defined and consistent (round half up vs floor)
## Reconciliation
- [ ] Reconciliation logic matches payment provider's settlement model
- [ ] Discrepancy thresholds defined (when to auto-adjust vs flag for manual review)
- [ ] Reconciliation runs are idempotent
## Refund State Machine
- [ ] All refund states defined (pending, processing, completed, failed)
- [ ] Transition rules enforced (can't refund a refunded payment)
- [ ] Partial refund logic correct (remaining refundable amount tracked)
## Third-Party Timeout
- [ ] Payment provider timeout handled (request timed out ≠ payment failed)
- [ ] Retry strategy for querying payment status
- [ ] Circuit breaker or backoff for provider outages
@@ -0,0 +1,28 @@
# Permission Module — Manual Review Checklist
## RBAC Model
- [ ] Roles and permissions clearly defined and documented
- [ ] No permission inheritance cycles
- [ ] Super admin role cannot be accidentally assigned to regular users
- [ ] Role changes are logged
## Authorization Enforcement
- [ ] Every protected endpoint checks permissions (not just login state)
- [ ] Resource-level permissions: user can only access own data
- [ ] Admin endpoints segregated from user endpoints (different middleware chain)
## Horizontal Escalation
- [ ] User A cannot access User B's resources by changing ID in request
- [ ] Resource ownership verified on every request (not just at list level)
- [ ] Bulk operations check permissions per-item, not just the batch endpoint
## Vertical Escalation
- [ ] Regular users cannot perform admin actions
- [ ] Role checks happen server-side (not trusting client-claimed role)
- [ ] Privileged operations require re-authentication
## Token Strategy
- [ ] Access token: short-lived (15-60 min)
- [ ] Refresh token: long-lived, stored securely (httpOnly cookie)
- [ ] Token rotation on refresh (old refresh token invalidated)
- [ ] Token revocation mechanism (logout invalidates all user tokens)
@@ -0,0 +1,45 @@
# Node.js Express Review Checklist
Extends the generic checklist with Node.js/Express-specific items.
## Interface Layer (Express Routes)
- [ ] Request validation middleware (Joi, Zod, express-validator)
- [ ] Response format consistent across all routes
- [ ] `express.json()` with size limit configured
- [ ] Route handlers are async with try/catch or wrapped with async handler
## Business Layer
- [ ] Business logic in service modules, not in route handlers
- [ ] Dependency injection or factory pattern for testability
- [ ] Config loaded from environment, not hardcoded
## Data Layer (Sequelize / Prisma / Knex)
- [ ] Sequelize: eager loading uses `include` with proper scoping
- [ ] Prisma: `select` or `include` to avoid over-fetching
- [ ] Raw queries always parameterized (`$1`, `?` placeholders)
- [ ] Connection pool configured (`max`, `min`, `idleTimeoutMillis`)
## Error Handling
- [ ] Global error handler middleware `(err, req, res, next)`
- [ ] Async route handlers wrapped (express-async-errors or manual wrapper)
- [ ] Error responses never expose stack traces in production
- [ ] `uncaughtException` and `unhandledRejection` handlers
## Security
- [ ] `helmet` middleware configured
- [ ] `cors` with specific origin allowlist
- [ ] `express-rate-limit` on auth and sensitive endpoints
- [ ] `httpOnly`, `secure`, `sameSite` flags on cookies
- [ ] No `eval()` or `Function()` with user input
## Performance
- [ ] Compression middleware (`compression`)
- [ ] Database queries have limits and pagination
- [ ] Heavy operations offloaded to worker threads or queue
- [ ] Static assets served via CDN or reverse proxy, not Express
@@ -0,0 +1,51 @@
# Python Django Review Checklist
Extends the generic checklist with Django-specific items.
## Interface Layer (Django Views / DRF)
- [ ] DRF: Serializers validate all input fields explicitly
- [ ] DRF: `serializer.is_valid(raise_exception=True)` used consistently
- [ ] DRF: Permission classes set on every ViewSet
- [ ] Function-based views have `@require_http_methods` decorator
- [ ] Django forms have `clean_*` methods for custom validation
## Business Layer
- [ ] Business logic in services/managers, not in views
- [ ] `transaction.atomic()` wraps multi-table writes
- [ ] `select_for_update()` used for pessimistic locking where needed
- [ ] `@transaction.atomic` decorator on methods with multiple DB writes
## Data Layer (Django ORM)
- [ ] `select_related()` for foreign key access in loops
- [ ] `prefetch_related()` for many-to-many access in loops
- [ ] `QuerySet.exists()` instead of `len(queryset)` for existence checks
- [ ] `QuerySet.count()` instead of `len(queryset)` for counts
- [ ] `QuerySet.only()` or `defer()` for large tables with unused columns
- [ ] Migrations are generated and reviewed (`makemigrations` + `check --deploy`)
## Error Handling
- [ ] Custom exception middleware or DRF exception handler
- [ ] `DEBUG = False` in production (prevents stack trace leaks)
- [ ] `LOGGING` configured with appropriate levels per module
- [ ] Sentry or similar error tracking integrated
## Security (Django)
- [ ] `SECRET_KEY` not hardcoded — loaded from environment
- [ ] `ALLOWED_HOSTS` properly restricted
- [ ] `SECURE_SSL_REDIRECT`, `SESSION_COOKIE_SECURE`, `CSRF_COOKIE_SECURE` enabled
- [ ] `X_FRAME_OPTIONS`, `SECURE_HSTS_SECONDS` set
- [ ] CSRF middleware enabled (default)
- [ ] File upload: `FILE_UPLOAD_MAX_MEMORY_SIZE` and validation
## Performance
- [ ] Celery tasks for async operations with proper retry config
- [ ] Django cache framework used with appropriate backend (Redis/Memcached)
- [ ] Database indexes on filtered/sorted fields
- [ ] `iterator()` for large querysets to avoid memory issues
- [ ] `bulk_create()` / `bulk_update()` for batch operations
@@ -0,0 +1,47 @@
# Python FastAPI Review Checklist
Extends the generic checklist with FastAPI-specific items.
## Interface Layer (FastAPI Routes)
- [ ] Pydantic models used for request/response schemas
- [ ] Pydantic validators (`@validator`, `@field_validator`) for custom logic
- [ ] `response_model` specified on all endpoints
- [ ] Query/Path parameters have `title`, `description`, `examples`
- [ ] `status_code` set explicitly on non-200 responses
- [ ] Dependency injection used for shared logic (auth, DB session)
## Business Layer
- [ ] Business logic separated from route handlers
- [ ] `Depends(get_db)` pattern for database session management
- [ ] Background tasks (`BackgroundTasks`) used for non-blocking operations
## Data Layer (SQLAlchemy / asyncpg)
- [ ] SQLAlchemy: session management via dependency injection
- [ ] SQLAlchemy: `selectinload()` / `joinedload()` for eager loading
- [ ] SQLAlchemy async: proper async session usage (`AsyncSession`)
- [ ] Raw SQL: always parameterized, never f-string interpolation
## Error Handling
- [ ] Custom exception handlers registered (`@app.exception_handler`)
- [ ] HTTPException with appropriate status codes
- [ ] Validation errors return structured response (Pydantic error format)
- [ ] Unhandled exceptions caught by global handler
## Security
- [ ] `CORSMiddleware` with specific origins, not `allow_origins=["*"]`
- [ ] OAuth2 / JWT integration via FastAPI security utilities
- [ ] `Security()` or `Depends()` for auth checks (not manual header parsing)
- [ ] Rate limiting middleware (e.g., slowapi)
- [ ] Secrets loaded from environment or secret manager
## Performance
- [ ] Async endpoints (`async def`) where I/O-bound
- [ ] `httpx.AsyncClient` with connection pooling for external API calls
- [ ] Response compression middleware (`GZipMiddleware`)
- [ ] Database connection pool size tuned
@@ -0,0 +1,211 @@
<!DOCTYPE html>
<html lang="zh-CN">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Code Review Report</title>
<style>
:root {
--bg: #1a1b1e;
--surface: #25262b;
--border: #373a40;
--text: #c1c2c5;
--text-muted: #909296;
--accent: #5c7cfa;
--success: #51cf66;
--warning: #fcc419;
--danger: #ff6b6b;
--info: #339af0;
}
* { margin: 0; padding: 0; box-sizing: border-box; }
body {
font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, 'Helvetica Neue', Arial, sans-serif;
background: var(--bg);
color: var(--text);
padding: 32px;
line-height: 1.6;
}
.container { max-width: 960px; margin: 0 auto; }
.header {
background: var(--surface);
border: 1px solid var(--border);
border-radius: 12px;
padding: 24px 32px;
margin-bottom: 24px;
}
.header h1 {
font-size: 22px;
color: #fff;
margin-bottom: 16px;
display: flex;
align-items: center;
gap: 10px;
}
.meta-grid {
display: grid;
grid-template-columns: repeat(auto-fit, minmax(200px, 1fr));
gap: 8px;
font-size: 14px;
}
.meta-item { display: flex; gap: 8px; }
.meta-label { color: var(--text-muted); min-width: 80px; }
.meta-value { color: #fff; }
.verdict { font-weight: 700; font-size: 18px; }
.verdict.pass { color: var(--success); }
.verdict.fail { color: var(--danger); }
.section {
background: var(--surface);
border: 1px solid var(--border);
border-radius: 12px;
padding: 24px 32px;
margin-bottom: 20px;
}
.section h2 {
font-size: 16px;
color: #fff;
margin-bottom: 16px;
padding-bottom: 8px;
border-bottom: 1px solid var(--border);
text-transform: uppercase;
letter-spacing: 0.5px;
}
.layer-grid { display: grid; grid-template-columns: repeat(auto-fit, minmax(140px, 1fr)); gap: 8px; }
.layer-item { display: flex; justify-content: space-between; padding: 6px 0; font-size: 14px; }
.layer-item .status { font-weight: 600; }
.status-clean { color: var(--success); }
.status-issues { color: var(--warning); }
.status-na { color: var(--text-muted); }
table { width: 100%; border-collapse: collapse; font-size: 14px; }
th, td { text-align: left; padding: 8px 12px; border-bottom: 1px solid var(--border); }
th { color: var(--text-muted); font-weight: 500; text-transform: uppercase; font-size: 12px; letter-spacing: 0.5px; }
td { color: var(--text); }
.metric-warn { color: var(--warning); font-weight: 600; }
.metric-good { color: var(--success); }
.summary-row { display: flex; gap: 16px; flex-wrap: wrap; }
.summary-card {
flex: 1; min-width: 140px; padding: 16px; border-radius: 8px; text-align: center;
border: 1px solid var(--border);
}
.summary-card .num { font-size: 28px; font-weight: 700; }
.summary-card .label { font-size: 12px; color: var(--text-muted); margin-top: 4px; }
.card-ready .num { color: var(--success); }
.card-needsfix .num { color: var(--warning); }
.card-unusable .num { color: var(--danger); }
.severity-row { display: flex; gap: 16px; flex-wrap: wrap; margin-bottom: 16px; }
.severity-badge {
display: inline-flex; align-items: center; gap: 6px;
padding: 4px 12px; border-radius: 20px; font-size: 13px; font-weight: 600;
}
.sev-blocker { background: rgba(255,107,107,0.15); color: var(--danger); }
.sev-major { background: rgba(252,196,25,0.15); color: var(--warning); }
.sev-minor { background: rgba(52,154,240,0.15); color: var(--info); }
.issue {
padding: 12px 16px; margin-bottom: 8px; border-radius: 8px;
border-left: 4px solid; font-size: 14px;
}
.issue-blocker { background: rgba(255,107,107,0.08); border-left-color: var(--danger); }
.issue-major { background: rgba(252,196,25,0.08); border-left-color: var(--warning); }
.issue-minor { background: rgba(52,154,240,0.08); border-left-color: var(--info); }
.issue .tag {
display: inline-block; padding: 1px 8px; border-radius: 4px;
font-size: 11px; font-weight: 600; margin-right: 8px;
}
.issue .tag-blocker { background: rgba(255,107,107,0.2); color: var(--danger); }
.issue .tag-major { background: rgba(252,196,25,0.2); color: var(--warning); }
.issue .tag-minor { background: rgba(52,154,240,0.2); color: var(--info); }
.issue .category { color: var(--accent); font-weight: 500; }
.issue .location { color: var(--text-muted); font-size: 12px; }
.issue .fix { margin-top: 4px; padding: 6px 10px; background: rgba(92,124,250,0.1); border-radius: 4px; font-size: 13px; color: #dde; }
.manual-item { padding: 8px 0; font-size: 14px; }
.manual-item::before { content: '→ '; color: var(--warning); }
.accum-item { padding: 6px 0; font-size: 14px; }
.accum-item::before { content: '• '; color: var(--accent); }
.footer { text-align: center; color: var(--text-muted); font-size: 12px; margin-top: 32px; }
</style>
</head>
<body>
<div class="container">
<!-- Header -->
<div class="header">
<h1>🔍 Backend Code Review Report</h1>
<div class="meta-grid">
<div class="meta-item"><span class="meta-label">Scope</span><span class="meta-value">{{SCOPE}}</span></div>
<div class="meta-item"><span class="meta-label">Tier</span><span class="meta-value">{{TIER}}</span></div>
<div class="meta-item"><span class="meta-label">Time</span><span class="meta-value">{{TIMESTAMP}}</span></div>
<div class="meta-item"><span class="meta-label">Files</span><span class="meta-value">{{FILES}}</span></div>
<div class="meta-item"><span class="meta-label">Baseline</span><span class="meta-value">{{BASELINE}}</span></div>
<div class="meta-item">
<span class="meta-label">Verdict</span>
<span class="meta-value verdict {{VERDICT_CLASS}}">{{VERDICT}}</span>
</div>
</div>
</div>
<!-- Layer 1 -->
<div class="section">
<h2>Layer 1 · Chain Decomposition</h2>
<div class="layer-grid">
<div class="layer-item"><span>Interface</span><span class="status {{L1_INTERFACE_CLASS}}">{{L1_INTERFACE}}</span></div>
<div class="layer-item"><span>Business</span><span class="status {{L1_BUSINESS_CLASS}}">{{L1_BUSINESS}}</span></div>
<div class="layer-item"><span>Data</span><span class="status {{L1_DATA_CLASS}}">{{L1_DATA}}</span></div>
<div class="layer-item"><span>Utility</span><span class="status {{L1_UTILITY_CLASS}}">{{L1_UTILITY}}</span></div>
<div class="layer-item"><span>Error Handling</span><span class="status {{L1_ERROR_CLASS}}">{{L1_ERROR}}</span></div>
<div class="layer-item"><span>Security</span><span class="status {{L1_SECURITY_CLASS}}">{{L1_SECURITY}}</span></div>
<div class="layer-item"><span>Performance</span><span class="status {{L1_PERFORMANCE_CLASS}}">{{L1_PERFORMANCE}}</span></div>
</div>
</div>
<!-- Layer 2 -->
<div class="section">
<h2>Layer 2 · Quantitative Metrics</h2>
<table>
<thead><tr><th>Metric</th><th>Value</th></tr></thead>
<tbody>
<tr><td>Requirement Coverage</td><td class="{{REQ_COV_CLASS}}">{{REQ_COVERAGE}}</td></tr>
<tr><td>Logic Alignment</td><td>{{LOGIC_ALIGNMENT}}</td></tr>
<tr><td>Exception Branch Coverage</td><td class="{{EXC_COV_CLASS}}">{{EXC_COVERAGE}}</td></tr>
<tr><td>SQL Performance Risk</td><td class="{{SQL_RISK_CLASS}}">{{SQL_RISK}}</td></tr>
<tr><td>Code Redundancy Rate</td><td>{{REDUNDANCY_RATE}}</td></tr>
<tr><td>Vulnerability Risk Rate</td><td class="{{VULN_RISK_CLASS}}">{{VULN_RISK}}</td></tr>
<tr><td>High-Risk Scenario Coverage</td><td class="{{HRISK_CLASS}}">{{HRISK_COVERAGE}}</td></tr>
</tbody>
</table>
</div>
<!-- Classification -->
<div class="section">
<h2>Classification</h2>
<div class="summary-row">
<div class="summary-card card-ready"><div class="num">{{READY_COUNT}}</div><div class="label">Ready</div></div>
<div class="summary-card card-needsfix"><div class="num">{{NEEDS_FIX_COUNT}}</div><div class="label">Needs Fix</div></div>
<div class="summary-card card-unusable"><div class="num">{{UNUSABLE_COUNT}}</div><div class="label">Unusable</div></div>
</div>
</div>
<!-- Severity Summary -->
<div class="section">
<h2>Severity Summary</h2>
<div class="severity-row">
<span class="severity-badge sev-blocker">🔴 Blocker: {{BLOCKER_COUNT}}</span>
<span class="severity-badge sev-major">🟡 Major: {{MAJOR_COUNT}}</span>
<span class="severity-badge sev-minor">🔵 Minor: {{MINOR_COUNT}}</span>
</div>
{{ISSUES_LIST}}
</div>
<!-- Manual Review Required -->
{{MANUAL_REVIEW_SECTION}}
<!-- Knowledge Accumulation -->
{{ACCUMULATION_SECTION}}
<!-- Footer -->
<div class="footer">
Generated by code-review skill · {{TIMESTAMP}}
</div>
</div>
</body>
</html>
@@ -0,0 +1,95 @@
# Generic Code Review Checklist
Language-agnostic checklist covering the seven code categories across eight quality metrics. Used when no language-specific checklist matches.
---
## 1. Interface Layer
- [ ] All public endpoints have explicit parameter validation
- [ ] Response format is consistent across endpoints
- [ ] HTTP status codes are semantically correct (2xx success, 4xx client error, 5xx server error)
- [ ] Rate limiting considered for public-facing endpoints
- [ ] Request/response DTOs are complete (no partial exposure of internal models)
- [ ] Content-Type headers are set correctly
- [ ] Pagination implemented for list endpoints
## 2. Business Layer
- [ ] Business logic matches requirements (check each rule)
- [ ] State machines have all transitions defined (no impossible states)
- [ ] Idempotency designed for non-idempotent operations (create, update with side effects)
- [ ] Distributed lock usage reviewed for correctness (lock key, timeout, release)
- [ ] Business exceptions are domain-specific, not generic
- [ ] No business logic leaks into interface or data layer
## 3. Data Layer
- [ ] All queries use parameterized statements (no string concatenation)
- [ ] Every query has appropriate indexes (check EXPLAIN output)
- [ ] Transaction boundaries are correct (ACID where needed, rollback on error)
- [ ] No full table scans on large tables
- [ ] Batch operations used for bulk inserts/updates
- [ ] Connection pooling configured
- [ ] N+1 queries eliminated (use JOIN or batch fetch)
- [ ] Sensitive data encrypted at rest if required
## 4. Utility Layer
- [ ] All utility functions validate inputs
- [ ] Utility functions are pure (no side effects) unless explicitly documented
- [ ] Error states return explicit values, not null/undefined without documentation
- [ ] Date/time handling uses consistent timezone approach
- [ ] String operations handle Unicode and edge cases (empty, very long)
## 5. Error Handling
- [ ] Exceptions are categorized (not all caught as generic Exception/Error)
- [ ] Every external call (DB, API, file I/O) has a fallback or error boundary
- [ ] Error messages returned to clients do not leak internal details
- [ ] Retry strategy exists for transient failures (network, deadlock)
- [ ] Logging includes enough context for debugging (request ID, user, operation)
- [ ] Circuit breaker considered for critical external dependencies
## 6. Security
- [ ] Authentication enforced on all protected endpoints
- [ ] Authorization checked per operation, not just per endpoint
- [ ] User input validated and sanitized (XSS, injection prevention)
- [ ] Sensitive data (passwords, tokens, PII) never logged or exposed in responses
- [ ] CSRF protection for state-changing operations (if cookie-based auth)
- [ ] CORS configured restrictively, not wildcard
- [ ] Secrets (API keys, DB credentials) loaded from environment, never hardcoded
- [ ] File upload has size limits and type validation
## 7. Performance
- [ ] No N+1 query patterns
- [ ] Appropriate caching strategy (with invalidation logic)
- [ ] Heavy operations are async where possible
- [ ] Database queries have reasonable LIMIT clauses
- [ ] Large responses support pagination or streaming
- [ ] Connection pooling and timeouts configured
- [ ] Memory usage considered for large datasets
---
## Quantitative Metrics Reference
| Metric | Check |
|--------|-------|
| Requirement Coverage | Every requirement item mapped to ≥1 code location |
| Logic Alignment | Business rules implemented exactly as specified |
| Exception Branch Coverage | ≥1 error path for every 2 happy paths (baseline) |
| SQL Performance Risk | No full scans on tables > 10k rows; queries have indexes |
| Code Redundancy Rate | < 10% of code is copy-pasted (same logic in ≥3 places) |
| Vulnerability Risk Rate | 0 known vulnerability patterns (OWASP Top 10) |
| High-Risk Scenario Coverage | Concurrency, transactions, idempotency addressed for stateful ops |
---
## Classification Rules
- **Ready:** All 🔴 and 🟡 items passed. 🔵 items optional.
- **Needs Fix:** 🟡 items found but fixable. No 🔴 items.
- **Unusable:** 🔴 items found or logic fundamentally incorrect.