526 lines
13 KiB
Markdown
526 lines
13 KiB
Markdown
|
|
|||
|
|
review
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
# Code Review Prompt (improved)
|
|||
|
|
|
|||
|
|
**Goal:** Provide a rigorous, actionable review that balances correctness, security, and maintainability for the following code.
|
|||
|
|
|
|||
|
|
## Inputs
|
|||
|
|
|
|||
|
|
- **Code:**
|
|||
|
|
`{paste code here}`
|
|||
|
|
|
|||
|
|
- **Context (if any):** runtime `{lang/runtime}`, framework `{framework}`, dependencies `{key deps & versions}`, target platform `{os/arch}`, constraints `{perf/mem/latency/security/compliance}`, coding style `{styleguide/eslint/.editorconfig}`, known requirements `{tickets/PRD refs}`.
|
|||
|
|
|
|||
|
|
|
|||
|
|
## Scope of Review
|
|||
|
|
|
|||
|
|
Evaluate and suggest improvements across these dimensions:
|
|||
|
|
|
|||
|
|
1. **Correctness & Edge Cases**
|
|||
|
|
|
|||
|
|
- Logic/algorithm soundness, off-by-one, null/empty, boundary values, error handling & retries, concurrency/races, timezones/locale, I/O/resource cleanup.
|
|||
|
|
|
|||
|
|
2. **Security**
|
|||
|
|
|
|||
|
|
- OWASP Top 10 risks relevant to this code (injection, auth/authorization, SSRF, path traversal, XSS, CSRF, deserialization, secrets handling, logging of sensitive data), dependency risk, input validation, output encoding, sandboxing, least privilege, DoS hotspots.
|
|||
|
|
|
|||
|
|
3. **Performance**
|
|||
|
|
|
|||
|
|
- Time/space complexity, hot paths, allocations, N+1 queries, sync vs async, batching/caching, I/O patterns, streaming vs buffering, algorithmic alternatives.
|
|||
|
|
|
|||
|
|
4. **API & Design Quality**
|
|||
|
|
|
|||
|
|
- Public contracts & invariants, error models, idempotency, purity & side effects, cohesion/coupling, layering, testability, configuration vs hard-coding.
|
|||
|
|
|
|||
|
|
5. **Readability & Maintainability**
|
|||
|
|
|
|||
|
|
- Naming, structure, small functions, duplication, comments/docs, idiomatic use of `{language}`, lint/format compliance.
|
|||
|
|
|
|||
|
|
|
|||
|
|
## Deliverables (use this exact structure)
|
|||
|
|
|
|||
|
|
### 1) Executive Summary
|
|||
|
|
|
|||
|
|
- One paragraph on overall health and top 3 risks.
|
|||
|
|
|
|||
|
|
|
|||
|
|
### 2) Findings Table
|
|||
|
|
|
|||
|
|
Provide a table with: **ID | Severity (High/Med/Low) | Category | Symptom | Why it matters | Evidence (line refs) | Fix summary**
|
|||
|
|
|
|||
|
|
### 3) Patch Suggestions
|
|||
|
|
|
|||
|
|
For each High/Med item, include a **minimal diff** or **before/after** snippet:
|
|||
|
|
|
|||
|
|
```diff
|
|||
|
|
{target file path}
|
|||
|
|
- {problematic code}
|
|||
|
|
+ {improved code}
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
Explain the trade-offs and why the fix is correct.
|
|||
|
|
|
|||
|
|
### 4) Tests to Add
|
|||
|
|
|
|||
|
|
List concrete test cases (names + intent). Include edge values and failure paths.
|
|||
|
|
|
|||
|
|
- Unit: `{TestName_Should...}`
|
|||
|
|
|
|||
|
|
- Integration: `{Scenario_When..._Then...}`
|
|||
|
|
|
|||
|
|
- Property/Fuzz (if applicable): input domains & invariants.
|
|||
|
|
|
|||
|
|
|
|||
|
|
### 5) Performance Notes
|
|||
|
|
|
|||
|
|
- Estimated complexity and bottlenecks.
|
|||
|
|
|
|||
|
|
- Quick wins (e.g., cache/batch/stream) and expected impact.
|
|||
|
|
|
|||
|
|
|
|||
|
|
### 6) Security Checklist
|
|||
|
|
|
|||
|
|
- Inputs validated? Output encoded? Secrets sourced from vault? Least privilege? Safe defaults? Rate limiting? Logging PII redaction?
|
|||
|
|
|
|||
|
|
|
|||
|
|
### 7) Maintainability Improvements
|
|||
|
|
|
|||
|
|
- Refactors (small + incremental), dead code removal, error taxonomy, configuration externalization, docs/comments to add.
|
|||
|
|
|
|||
|
|
|
|||
|
|
### 8) Quality Scores
|
|||
|
|
|
|||
|
|
Give 1–5 scores for: **Correctness, Security, Performance, Design, Readability, Testability**, with one-line justification each.
|
|||
|
|
|
|||
|
|
## Constraints
|
|||
|
|
|
|||
|
|
- Prefer **minimal, targeted changes** over large rewrites.
|
|||
|
|
|
|||
|
|
- Match existing project style and patterns.
|
|||
|
|
|
|||
|
|
- If context is missing, **state assumptions explicitly** and proceed.
|
|||
|
|
|
|||
|
|
- Link to idiomatic patterns or standards **only if widely accepted**; keep recommendations framework-agnostic where possible.
|
|||
|
|
|
|||
|
|
|
|||
|
|
## Output Format
|
|||
|
|
|
|||
|
|
Return **only** the sections 1–8 above in Markdown. Keep code blocks self-contained and compilable where possible.
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
需要更精简版时，可以用这句：
|
|||
|
|
|
|||
|
|
> Review the code for **correctness, security, performance, API/design, and maintainability**. Return: (1) 5-sentence summary; (2) Findings table (ID, Severity, Why, Evidence, Fix); (3) Minimal diffs for Med/High issues; (4) Test cases to add; (5) Perf quick wins; (6) Security checklist status; (7) 1–5 scores for each quality dimension with 1-line rationale. Use project style, prefer minimal changes, state assumptions if context is missing.
|
|||
|
|
|
|||
|
|
|
|||
|
|
|
|||
|
|
|
|||
|
|
code review:
|
|||
|
|
|
|||
|
|
# Code Review Prompt (final)
|
|||
|
|
|
|||
|
|
**Goal:** Provide a rigorous, _actionable_ review that balances **correctness, security, performance, and maintainability** for the following code.
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## Inputs
|
|||
|
|
|
|||
|
|
- **Code:**
|
|||
|
|
|
|||
|
|
```text
|
|||
|
|
{paste code here}
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
- **Context (optional but recommended):**
|
|||
|
|
|
|||
|
|
- runtime: `{lang/runtime}`
|
|||
|
|
|
|||
|
|
- framework: `{framework}`
|
|||
|
|
|
|||
|
|
- key dependencies & versions: `{deps & versions}`
|
|||
|
|
|
|||
|
|
- target platform: `{os/arch}`
|
|||
|
|
|
|||
|
|
- constraints: `{perf/mem/latency/security/compliance}`
|
|||
|
|
|
|||
|
|
- coding style: `{styleguide/eslint/.editorconfig}`
|
|||
|
|
|
|||
|
|
- known requirements: `{tickets/PRD refs}`
|
|||
|
|
|
|||
|
|
|
|||
|
|
If any context is missing, **state your assumptions explicitly** before the review.
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## Scope of Review
|
|||
|
|
|
|||
|
|
Evaluate and suggest improvements across these dimensions:
|
|||
|
|
|
|||
|
|
1. **Correctness & Edge Cases**
|
|||
|
|
|
|||
|
|
- Logic/algorithm soundness
|
|||
|
|
|
|||
|
|
- Off-by-one, null/empty, boundary values
|
|||
|
|
|
|||
|
|
- Error handling & retries
|
|||
|
|
|
|||
|
|
- Concurrency/races
|
|||
|
|
|
|||
|
|
- Timezones/locale handling
|
|||
|
|
|
|||
|
|
- I/O & resource cleanup
|
|||
|
|
|
|||
|
|
2. **Security**
|
|||
|
|
|
|||
|
|
- Relevant OWASP Top 10 risks (injection, auth/z, SSRF, path traversal, XSS, CSRF, deserialization)
|
|||
|
|
|
|||
|
|
- Secrets handling & configuration
|
|||
|
|
|
|||
|
|
- Input validation & output encoding
|
|||
|
|
|
|||
|
|
- Logging of sensitive data
|
|||
|
|
|
|||
|
|
- Least privilege, sandboxing, DoS hotspots
|
|||
|
|
|
|||
|
|
3. **Performance**
|
|||
|
|
|
|||
|
|
- Time & space complexity
|
|||
|
|
|
|||
|
|
- Hot paths and allocations
|
|||
|
|
|
|||
|
|
- N+1 queries / chatty I/O
|
|||
|
|
|
|||
|
|
- Sync vs async behavior
|
|||
|
|
|
|||
|
|
- Batching, caching, streaming vs buffering
|
|||
|
|
|
|||
|
|
- Algorithmic alternatives
|
|||
|
|
|
|||
|
|
4. **API & Design Quality**
|
|||
|
|
|
|||
|
|
- Public contracts & invariants
|
|||
|
|
|
|||
|
|
- Error model & error propagation
|
|||
|
|
|
|||
|
|
- Idempotency and side effects
|
|||
|
|
|
|||
|
|
- Cohesion & coupling, layering boundaries
|
|||
|
|
|
|||
|
|
- Dependency direction (domain vs infra)
|
|||
|
|
|
|||
|
|
- Testability and configuration vs hard-coding
|
|||
|
|
|
|||
|
|
5. **Readability & Maintainability**
|
|||
|
|
|
|||
|
|
- Naming and intent clarity
|
|||
|
|
|
|||
|
|
- Function/module size and structure
|
|||
|
|
|
|||
|
|
- Duplication vs reuse
|
|||
|
|
|
|||
|
|
- Comments/docs (where needed)
|
|||
|
|
|
|||
|
|
- Idiomatic use of `{language}`
|
|||
|
|
|
|||
|
|
- Lint/format compliance
|
|||
|
|
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## Deliverables (use this exact structure)
|
|||
|
|
|
|||
|
|
### 1) Executive Summary
|
|||
|
|
|
|||
|
|
- One short paragraph on overall health.
|
|||
|
|
|
|||
|
|
- List the **top 3 risks or opportunities** (bullets).
|
|||
|
|
|
|||
|
|
|
|||
|
|
### 2) Findings Table
|
|||
|
|
|
|||
|
|
Provide a table with:
|
|||
|
|
|
|||
|
|
- **ID** – short stable identifier (e.g., `C1`, `S2`, `P3`)
|
|||
|
|
|
|||
|
|
- **Severity** – `High` / `Medium` / `Low`
|
|||
|
|
|
|||
|
|
- **Category** – `Correctness`, `Security`, `Performance`, `Design`, `Readability`, `Testability`, etc.
|
|||
|
|
|
|||
|
|
- **Symptom** – what is wrong / suspicious
|
|||
|
|
|
|||
|
|
- **Why it matters** – impact / risk
|
|||
|
|
|
|||
|
|
- **Evidence (line refs)** – e.g., `file.go:42-57`
|
|||
|
|
|
|||
|
|
- **Fix summary** – 1–2 line suggested direction
|
|||
|
|
|
|||
|
|
|
|||
|
|
Example:
|
|||
|
|
|
|||
|
|
|ID|Severity|Category|Symptom|Why it matters|Evidence|Fix summary|
|
|||
|
|
|---|---|---|---|---|---|---|
|
|||
|
|
|C1|High|Correctness|Possible nil deref on error path|Can cause runtime panic in production|`handler.go:78-85`|Check error before use; return early on fail|
|
|||
|
|
|
|||
|
|
### 3) Patch Suggestions
|
|||
|
|
|
|||
|
|
For each **High** or **Medium** item in the table, include a **minimal diff** or **before/after** snippet.
|
|||
|
|
|
|||
|
|
```diff
|
|||
|
|
{target file path}
|
|||
|
|
- {problematic code}
|
|||
|
|
+ {improved code}
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
- Keep patches **local and incremental**, not full rewrites.
|
|||
|
|
|
|||
|
|
- Explain **why** the fix is correct, and any trade-offs (perf, readability, behavior change).
|
|||
|
|
|
|||
|
|
|
|||
|
|
### 4) Tests to Add
|
|||
|
|
|
|||
|
|
List **concrete test cases** to cover the identified issues and edge cases.
|
|||
|
|
|
|||
|
|
- Unit tests (with intent):
|
|||
|
|
|
|||
|
|
- `Test_{UnitName}_ShouldHandleEmptyInput` – verifies behavior when input is empty
|
|||
|
|
|
|||
|
|
- `Test_{FuncName}_ShouldReturnErrorOnTimeout` – covers timeout/failure path
|
|||
|
|
|
|||
|
|
- Integration tests:
|
|||
|
|
|
|||
|
|
- `{Scenario_When..._Then...}` – describe full flows: external calls, DB, queues, etc.
|
|||
|
|
|
|||
|
|
- Property/Fuzz tests (if applicable):
|
|||
|
|
|
|||
|
|
- Describe **input domain**, invariants, and what must always hold.
|
|||
|
|
|
|||
|
|
|
|||
|
|
Where possible, map tests back to **Finding IDs** (e.g. “C1, S2”).
|
|||
|
|
|
|||
|
|
### 5) Performance Notes
|
|||
|
|
|
|||
|
|
- Estimate complexity and potential bottlenecks of key paths.
|
|||
|
|
|
|||
|
|
- Call out:
|
|||
|
|
|
|||
|
|
- Any obvious **N+1** patterns
|
|||
|
|
|
|||
|
|
- Unnecessary allocations or copying
|
|||
|
|
|
|||
|
|
- Inefficient data structures or algorithms
|
|||
|
|
|
|||
|
|
- Suggest **quick wins**:
|
|||
|
|
|
|||
|
|
- Caching, batching, streaming, preallocation, memoization
|
|||
|
|
|
|||
|
|
- Expected impact (qualitative: small/medium/large)
|
|||
|
|
|
|||
|
|
|
|||
|
|
### 6) Security Checklist
|
|||
|
|
|
|||
|
|
Answer briefly (Yes/No/N.A. + short note):
|
|||
|
|
|
|||
|
|
- Inputs validated at boundaries?
|
|||
|
|
|
|||
|
|
- Outputs properly encoded for their sinks (HTML/SQL/OS/etc.)?
|
|||
|
|
|
|||
|
|
- Auth & authorization checks present and correctly ordered?
|
|||
|
|
|
|||
|
|
- Secrets kept out of code (config, env, vault)?
|
|||
|
|
|
|||
|
|
- Least privilege for external resources (DB, queues, files)?
|
|||
|
|
|
|||
|
|
- Safe defaults (e.g., secure TLS, secure cookies, strict modes)?
|
|||
|
|
|
|||
|
|
- Rate limiting / throttling for expensive or exposed endpoints?
|
|||
|
|
|
|||
|
|
- Logs avoid PII/credential leakage; sensitive data redacted or omitted?
|
|||
|
|
|
|||
|
|
|
|||
|
|
Highlight any **High** severity gaps and link them to Findings IDs.
|
|||
|
|
|
|||
|
|
### 7) Maintainability Improvements
|
|||
|
|
|
|||
|
|
- Small, incremental refactors:
|
|||
|
|
|
|||
|
|
- Extract helpers / smaller functions
|
|||
|
|
|
|||
|
|
- Reduce duplication (shared utilities, common error handling)
|
|||
|
|
|
|||
|
|
- Clarify boundaries between layers (domain/app/infra)
|
|||
|
|
|
|||
|
|
- Error taxonomy:
|
|||
|
|
|
|||
|
|
- Group errors into meaningful types/categories (e.g., validation vs system vs external)
|
|||
|
|
|
|||
|
|
- Standardize error wrapping and messages
|
|||
|
|
|
|||
|
|
- Configuration:
|
|||
|
|
|
|||
|
|
- Externalize magic numbers/strings
|
|||
|
|
|
|||
|
|
- Centralize feature flags or switches
|
|||
|
|
|
|||
|
|
- Documentation:
|
|||
|
|
|
|||
|
|
- Add or update docstrings for non-obvious logic
|
|||
|
|
|
|||
|
|
- Brief README/ADR notes if design is non-trivial
|
|||
|
|
|
|||
|
|
|
|||
|
|
### 8) Quality Scores
|
|||
|
|
|
|||
|
|
Give **1–5** scores (5 = excellent, 1 = poor) with a **one-line justification** each:
|
|||
|
|
|
|||
|
|
- **Correctness:** `X/5` – `{short reason}`
|
|||
|
|
|
|||
|
|
- **Security:** `X/5` – `{short reason}`
|
|||
|
|
|
|||
|
|
- **Performance:** `X/5` – `{short reason}`
|
|||
|
|
|
|||
|
|
- **Design:** `X/5` – `{short reason}`
|
|||
|
|
|
|||
|
|
- **Readability:** `X/5` – `{short reason}`
|
|||
|
|
|
|||
|
|
- **Testability:** `X/5` – `{short reason}`
|
|||
|
|
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## Constraints
|
|||
|
|
|
|||
|
|
- Prefer **minimal, targeted changes** over big-bang rewrites.
|
|||
|
|
|
|||
|
|
- Match **existing project style and patterns** where visible.
|
|||
|
|
|
|||
|
|
- If context is missing, **state assumptions explicitly** and proceed.
|
|||
|
|
|
|||
|
|
- Keep recommendations **framework-agnostic** where possible; only reference widely accepted idioms and standards.
|
|||
|
|
|
|||
|
|
- When in doubt, **prioritize clarity and safety** over micro-optimizations.
|
|||
|
|
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## Output Format
|
|||
|
|
|
|||
|
|
Return **only** sections **1–8** above in Markdown when performing an actual review.
|
|||
|
|
Keep all code blocks self-contained and compilable where possible.
|
|||
|
|
|
|||
|
|
|
|||
|
|
|
|||
|
|
|
|||
|
|
|
|||
|
|
|
|||
|
|
----
|
|||
|
|
|
|||
|
|
# 可观测性 —— **4.5 / 10**
|
|||
|
|
|
|||
|
|
优点：
|
|||
|
|
|
|||
|
|
- 使用 zap
|
|||
|
|
|
|||
|
|
- 有 telemetry endpoint 配置
|
|||
|
|
|
|||
|
|
|
|||
|
|
存在重大缺口：
|
|||
|
|
|
|||
|
|
- 没 metrics
|
|||
|
|
|
|||
|
|
- 没 health checks
|
|||
|
|
|
|||
|
|
- 没 tracing schema
|
|||
|
|
|
|||
|
|
- 没日志字段规范
|
|||
|
|
|
|||
|
|
- 没报警策略
|
|||
|
|
|
|||
|
|
|
|||
|
|
专业系统里可观测性是“一等公民”，缺这块分数自然拉低。
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
# 4️⃣ 可靠性（Reliability & Fault Handling）—— **5.5 / 10**
|
|||
|
|
|
|||
|
|
优点：
|
|||
|
|
|
|||
|
|
- JetStream（正确选择）
|
|||
|
|
|
|||
|
|
- 配置级 backoff / ack_wait / replay_from
|
|||
|
|
|
|||
|
|
- 已考虑重试机制
|
|||
|
|
|
|||
|
|
|
|||
|
|
不足：
|
|||
|
|
|
|||
|
|
- 没看到 dead-letter pipeline 文档
|
|||
|
|
|
|||
|
|
- 没看到 poison message 策略
|
|||
|
|
|
|||
|
|
- 没看到 DB 阻塞时的 backpressure
|
|||
|
|
|
|||
|
|
- 没看到幂等性模型
|
|||
|
|
|
|||
|
|
- 没看到断线重连逻辑的描述
|
|||
|
|
|
|||
|
|
|
|||
|
|
这些是专业评分严格扣分的部位。
|
|||
|
|
|
|||
|
|
|
|||
|
|
|
|||
|
|
### 严格评审的缺失
|
|||
|
|
|
|||
|
|
- 没看到“dead-letter pipeline”定义
|
|||
|
|
|
|||
|
|
- 没看到“poison message”策略
|
|||
|
|
|
|||
|
|
- 没看到“持久化失败策略”
|
|||
|
|
|
|||
|
|
- 没看到“DB 降级”逻辑
|
|||
|
|
|
|||
|
|
- 没看到“幂等性策略”（特别关键）
|
|||
|
|
|
|||
|
|
- 没看到“重平衡策略”（consumer scaling）
|
|||
|
|
|
|||
|
|
- 没看到“高可用拓扑”（replicas 仅是 JetStream 层，服务自身无说明）
|
|||
|
|
|
|||
|
|
|
|||
|
|
按专业级评分，就是 **4/10**。
|
|||
|
|
|
|||
|
|
|
|||
|
|
这个维度是最严格的（专业评分里非常重要）。
|
|||
|
|
|
|||
|
|
### ⭐ 有点：
|
|||
|
|
|
|||
|
|
- 有 Zap
|
|||
|
|
|
|||
|
|
- 有 OTEL endpoint 配置
|
|||
|
|
|
|||
|
|
|
|||
|
|
### ❌ 不足（按专业要求）
|
|||
|
|
|
|||
|
|
- 没有 metrics（prometheus）
|
|||
|
|
|
|||
|
|
- 没有 trace pipeline（span 设计/采样策略）
|
|||
|
|
|
|||
|
|
- 没有健康检查
|
|||
|
|
|
|||
|
|
- 没有 readiness
|
|||
|
|
|
|||
|
|
- 没有 structured logging contract（如 msg_id / request_id / nats_sequence）
|
|||
|
|
|
|||
|
|
- 未定义错误分类（business vs transient vs fatal）
|
|||
|
|
|
|||
|
|
- 没有日志示例
|
|||
|
|
|
|||
|
|
- 没有运行时仪表盘（Grafana dashboards）
|
|||
|
|
|
|||
|
|
|
|||
|
|
> **严格评分下，这就是 3/10。**
|
|||
|
|
>
|
|||
|
|
|
|||
|
|
----
|