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。**
|
||
>
|
||
|
||
---- |