# Code Review And Quality

> 执行多维度代码评审。适用于合并任何改动之前，也适用于评审自己、其他 agent 或人类写出的代码。当你需要在改动进入主分支前从多个维度评估其质量时，就用这个 skill。

- Skill: `233i/code-review-and-quality` (Agent Skill)
- Install (CLI): `npx skillmds@latest add 233i/code-review-and-quality`
- Raw SKILL.md: https://api.skillmd.com/api/skills/233i/code-review-and-quality/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: AI & ML
- Author: 233i (https://skillmd.com/u/233i)
- Updated: 2026-09-22
- Page: https://skillmd.com/skills/233i/code-review-and-quality

---


# 代码评审与质量

## 概览

带质量门禁的多维度代码评审。每一项改动在合并前都必须经过评审，没有例外。评审覆盖五个维度：正确性、可读性、架构、安全性和性能。

**批准标准：** 当一个改动能够明确提升整体代码健康度时，就应该批准，即使它还不完美。完美代码不存在，目标是持续改进。不要因为“不是你会写出来的样子”就阻塞变更。只要它让代码库变得更好，并且遵守项目约定，就应该批准。

## 何时使用

- 合并任何 PR 或改动之前
- 完成功能实现之后
- 当另一个 agent 或模型产出了你需要评估的代码时
- 重构现有代码时
- 任意 bug 修复之后，同时评审修复本身和回归测试

## 五维评审

每次评审都要从以下维度检查代码：

### 1. 正确性

这段代码是否真的完成了它声称完成的事？

- 是否符合 spec 或任务要求？
- 边界情况是否处理了，例如 `null`、空值、临界值？
- 是否处理了错误路径，而不仅是 happy path？
- 所有测试是否通过？测试本身是否真的在测正确的东西？
- 是否有 off-by-one、竞态条件或状态不一致问题？

### 2. 可读性与简洁性

另一个工程师或 agent 是否能在不找作者解释的前提下看懂它？

- 名称是否清晰，是否符合项目约定？不要出现无上下文的 `temp`、`data`、`result`
- 控制流是否直白？避免嵌套三元表达式、深层回调
- 代码组织是否合理？相关内容是否放在一起，模块边界是否清晰
- 有没有“聪明过头”的技巧应该被简化？
- **能不能用更少的代码实现？** 1000 行才能做完 100 行就能做的事，就是失败
- **抽象是否配得上它的复杂度？** 没有第三个用例前，不要泛化
- 是否存在需要注释解释的非显然意图？不要注释显而易见的代码
- 有没有死代码痕迹，例如 `_unused`、兼容旧逻辑的 shim、`// removed` 一类注释？

### 3. 架构

这个改动是否符合系统设计？

- 它是沿用现有模式，还是引入了新模式？如果是新模式，理由是否充分？
- 是否保持了清晰的模块边界？
- 有没有本该共用却重复实现的代码？
- 依赖方向是否正确，有没有循环依赖？
- 抽象层级是否合适？既不过度设计，也不过度耦合

### 4. 安全性

更详细的安全指导见 `security-and-hardening`。这个改动会不会引入漏洞？

- 用户输入是否做了校验和清洗？
- Secrets 是否远离代码、日志和版本控制？
- 该做认证 / 授权检查的地方是否做了？
- SQL 查询是否参数化，避免字符串拼接？
- 输出是否编码以防 XSS？
- 依赖是否来自可信来源，且无已知漏洞？
- 来自外部的数据，例如 API、日志、用户内容、配置文件，是否都被当作不可信输入？
- 外部数据流在进入业务逻辑或渲染前，是否在系统边界被验证？

### 5. 性能

更详细的性能分析见 `performance-optimization`。这个改动是否引入性能问题？

- 有没有 N+1 查询模式？
- 有没有无界循环或无限制的数据抓取？
- 有没有该异步却写成同步的操作？
- UI 组件里有没有不必要的重复渲染？
- 列表接口是否缺少分页？
- 热路径上是否构造了大对象？

## 改动大小

小而聚焦的改动更容易评审、更快合并，也更安全。目标尺寸如下：

```
~100 lines changed   → Good. Reviewable in one sitting.
~300 lines changed   → Acceptable if it's a single logical change.
~1000 lines changed  → Too large. Split it.
```

**什么算“一项改动”：** 一个自洽的修改，只解决一件事，包含相关测试，并且提交后系统仍可工作。是一块功能切片，而不是整个功能。

**当改动过大时的拆分策略：**

| 策略 | 做法 | 适用场景 |
|----------|-----|------|
| **Stack** | 先提交小改动，再基于它继续下一层 | 串行依赖 |
| **By file group** | 按文件组拆分，方便不同 reviewer 处理 | 横跨多个关注面 |
| **Horizontal** | 先做共享代码 / 桩，再做使用方 | 分层架构 |
| **Vertical** | 按全栈功能切片继续拆小 | 功能开发 |

**什么情况下大改动还能接受：** 完整删除文件，或自动化重构这类 reviewer 只需验证意图、无需逐行看细节的改动。

**重构和功能改动要分开。** 一个同时做重构又新增行为的改动，本质上是两项改动，应分别提交。极小的清理类修改，例如变量重命名，可由 reviewer 酌情决定是否顺带接受。

## 改动说明

每一项改动都需要能独立存在于版本历史中的说明。

**第一行：** 简短、祈使句、可独立阅读。写 “Delete the FizzBuzz RPC”，不要写 “Deleting the FizzBuzz RPC”。这句话必须足够有信息量，让后来翻历史的人不看 diff 也知道发生了什么。

**正文：** 说明改了什么、为什么改。写上代码里看不出来的背景、决策和推理过程。有相关 bug 编号、基准测试或设计文档时一并链接。如果方案本身有短板，也要诚实说明。

**反模式：** “Fix bug”、“Fix build”、“Add patch”、“Moving code from A to B”、“Phase 1”、“Add convenience functions”。

## 评审流程

### 步骤 1：先理解上下文

看代码之前，先确认意图：

```
- What is this change trying to accomplish?
- What spec or task does it implement?
- What is the expected behavior change?
```

### 步骤 2：先看测试

测试能最快暴露意图与覆盖范围：

```
- Do tests exist for the change?
- Do they test behavior (not implementation details)?
- Are edge cases covered?
- Do tests have descriptive names?
- Would the tests catch a regression if the code changed?
```

### 步骤 3：再看实现

带着五个维度逐文件过：

```
For each file changed:
1. Correctness: Does this code do what the test says it should?
2. Readability: Can I understand this without help?
3. Architecture: Does this fit the system?
4. Security: Any vulnerabilities?
5. Performance: Any bottlenecks?
```

### 步骤 4：给问题分级

每条评论都标记严重程度，让作者知道哪些必须改，哪些只是建议：

| 前缀 | 含义 | 作者动作 |
|--------|---------|---------------|
| *(no prefix)* | Required change | 合并前必须处理 |
| **Critical:** | Blocks merge | 会阻塞合并，例如安全漏洞、数据丢失、功能损坏 |
| **Nit:** | Minor, optional | 小问题，可选，例如格式、个人风格偏好 |
| **Optional:** / **Consider:** | Suggestion | 值得考虑，但不是必须 |
| **FYI** | Informational only | 仅供参考，无需动作 |

这样可以避免作者把所有反馈都当成强制项，浪费时间在可选建议上。

### 步骤 5：检查验证是否可信

审一下作者给出的验证故事：

```
- What tests were run?
- Did the build pass?
- Was the change tested manually?
- Are there screenshots for UI changes?
- Is there a before/after comparison?
```

## 多模型评审模式

让不同模型承担不同评审视角：

```
Model A writes the code
    │
    ▼
Model B reviews for correctness and architecture
    │
    ▼
Model A addresses the feedback
    │
    ▼
Human makes the final call
```

这样能捕捉单一模型可能漏掉的问题，因为不同模型有不同盲点。

**给 review agent 的示例提示词：**
```
Review this code change for correctness, security, and adherence to
our project conventions. The spec says [X]. The change should [Y].
Flag any issues as Critical, Important, or Suggestion.
```

## 死代码卫生

任何重构或实现完成后，都检查一下有没有遗留孤儿代码：

1. 找出现在不可达或未使用的代码
2. 明确列出来
3. **删除前先问：** “这些现在未使用的元素是否要一并删除：[list]？”

不要把死代码留在仓库里，它会误导未来的读者和 agent。但也不要悄悄删除你拿不准的东西。拿不准，就问。

```
DEAD CODE IDENTIFIED:
- formatLegacyDate() in src/utils/date.ts — replaced by formatDate()
- OldTaskCard component in src/components/ — replaced by TaskCard
- LEGACY_API_URL constant in src/config.ts — no remaining references
→ Safe to remove these?
```

## 评审速度

慢评审会堵塞整个团队。为了做评审而短暂切换上下文的代价，远低于让别人等待的代价。

- **一个工作日内响应**，这是上限，不是目标
- **理想节奏：** 收到 review 请求后尽快响应，除非你正处于深度专注编码中。普通改动应能在一天内完成多轮 review
- **优先保证单次响应快**，比起很快给最终批准，更重要的是尽快给反馈，减少作者等待焦虑
- **如果改动太大：** 要求作者拆分，而不是硬着头皮 review 一个巨型 changeset

## 如何处理分歧

当 review 里出现争议时，按以下优先级处理：

1. **技术事实和数据** 高于个人意见和偏好
2. **Style guide** 是风格问题的最高裁决
3. **软件设计** 应按工程原则评估，而不是按个人口味
4. **代码库一致性** 只要不损害整体健康，就可以接受

**不要接受“我之后再清理”。** 经验表明，延期清理几乎不会真的发生。除非是真正的紧急情况，否则应要求在提交前清理完。如果本次改动确实无法顺手解决周边问题，也至少要求创建 bug 并由作者自我指派。

## 评审中的诚实

无论你评审的是自己、其他 agent 还是人类写的代码：

- **不要橡皮图章式通过。** 没有证据的 “LGTM” 对谁都没帮助。
- **不要弱化真实问题。** 明明是会打到生产上的 bug，却说成“可能有一点点小问题”，这是不诚实。
- **能量化就量化。** “这个 N+1 查询会让列表里每个元素多出约 50ms” 比 “这可能会慢” 更有价值。
- **面对明显有问题的做法要敢于指出。** 在 review 中一味迎合也是失败模式。如果实现有问题，就直接说明，并给替代方案。
- **接受 override。** 如果作者掌握完整上下文且不同意你的判断，可以接受。评论代码，不评论人，把人身评价重写为对代码本身的评价。

## 依赖纪律

代码评审也包括依赖评审：

**在添加任何依赖前：**
1. 现有技术栈能不能解决？很多时候其实可以
2. 这个依赖有多大？会不会影响 bundle
3. 它是否仍在活跃维护？看最近提交和 issue
4. 有没有已知漏洞？跑 `npm audit`
5. 许可证是否兼容项目？

**规则：** 优先使用标准库和现有工具，而不是新依赖。每一个依赖都是一项负债。

## 评审清单

```markdown
## Review: [PR/Change title]

### Context
- [ ] I understand what this change does and why

### Correctness
- [ ] Change matches spec/task requirements
- [ ] Edge cases handled
- [ ] Error paths handled
- [ ] Tests cover the change adequately

### Readability
- [ ] Names are clear and consistent
- [ ] Logic is straightforward
- [ ] No unnecessary complexity

### Architecture
- [ ] Follows existing patterns
- [ ] No unnecessary coupling or dependencies
- [ ] Appropriate abstraction level

### Security
- [ ] No secrets in code
- [ ] Input validated at boundaries
- [ ] No injection vulnerabilities
- [ ] Auth checks in place
- [ ] External data sources treated as untrusted

### Performance
- [ ] No N+1 patterns
- [ ] No unbounded operations
- [ ] Pagination on list endpoints

### Verification
- [ ] Tests pass
- [ ] Build succeeds
- [ ] Manual verification done (if applicable)

### Verdict
- [ ] **Approve** — Ready to merge
- [ ] **Request changes** — Issues must be addressed
```

## 常见自我安慰

| 自我安慰 | 现实 |
|---|---|
| “能跑就够了” | 能跑但不可读、不安全、架构不对的代码，会持续积累技术债。 |
| “这是我写的，我知道它是对的” | 作者对自己的假设最容易失明。每个改动都值得再被一双眼睛看一遍。 |
| “之后再清理” | Later 从来不会来。Review 就是质量门禁，清理应该在合并前完成，而不是合并后。 |
| “AI 生成的代码大概率没问题” | AI 代码更需要仔细审，而不是更少审。它经常自信、像样，但并不一定正确。 |
| “测试都过了，所以没问题” | 测试是必要条件，不是充分条件。它抓不到架构问题、安全问题或可读性问题。 |

## 危险信号

- PR 在没有任何 review 的情况下合并
- Review 只检查测试是否通过，而忽略其他维度
- 只有一句 “LGTM”，看不出真的审过
- 安全敏感改动没有安全视角 review
- PR 大到“根本无法认真评审”，这种必须拆分
- 修 bug 的 PR 却没有回归测试
- Review 评论没有严重级别，作者分不清哪些必须改
- 接受“我之后再修”，通常根本不会发生

## 验证

评审完成后，确认：

- [ ] 所有 Critical 问题已解决
- [ ] 所有 Important 问题要么已解决，要么有明确理由延期
- [ ] 测试通过
- [ ] 构建成功
- [ ] 验证故事已经记录清楚，包括改了什么、如何验证