# Review Code

> Use when reviewing code changes, PRs, diffs, or commits. 触发：审查、review、CR、看看这段代码、检查这个 PR、审一下。

- Skill: `ninehills/review-code` (Agent Skill)
- Install (CLI): `npx skillmds@latest add ninehills/review-code`
- Raw SKILL.md: https://api.skillmd.com/api/skills/ninehills/review-code/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- License: MIT
- Author: ninehills (https://skillmd.com/u/ninehills)
- Updated: 2026-09-10
- Page: https://skillmd.com/skills/ninehills/review-code

---


# 代码审查 (Code Review)

## Overview

基于 Google 工程实践整理的代码审查指南。**首要目的：确保代码库的整体代码健康状况随时间推移而改善。** 不存在"完美"代码，只有**更好**的代码——追求持续改善，而非追求完美。

评审者应在 CL 明确改善系统代码健康状态时予以通过，即使 CL 并不完美。

**三大原则：**

1. **技术事实和数据压倒意见和个人偏好。**
2. **风格问题上，风格指南是绝对权威。** 任何不在风格指南中的纯风格问题属于个人偏好，应保持与现有代码一致。
3. **软件设计几乎从来不是纯风格问题或个人偏好。** 基于底层原则衡量，而非仅凭个人意见。

## When to Use

**使用场景：**
- 审查代码变更、PR、diff、commit、patch
- 用户说"审查"、"review"、"CR"、"看看这段代码"、"检查这个 PR"、"审一下"
- 审计现有代码库或模块的代码质量
- 作为 `requesting-code-review` 审查阶段的参考标准

**不适用：**
- 纯文档变更、配置调整（除非涉及安全配置）
- 生成代码、数据文件、自动格式化的代码

**与 `requesting-code-review` 的区别：** 本技能定义审查**标准**和**方法**（审查什么、怎么看）。`requesting-code-review` 定义提交前的**自动化流水线**（安全扫描、基线对比、auto-fix 循环）。

## 审查总览

审查应覆盖以下维度：

| 维度 | 核心问题 |
|------|---------|
| **设计** | 代码设计是否合理、与系统适配？ |
| **功能** | 行为是否符合作者意图？对用户是否有益？ |
| **复杂度** | 能否更简单？未来其他开发者能否轻松理解和修改？ |
| **测试** | 是否有正确、设计良好的自动化测试？ |
| **命名** | 变量、类、方法等命名是否清晰？ |
| **注释** | 注释是否清晰有用？注释应解释**为什么**而非**是什么**。 |
| **风格** | 是否遵循风格指南？ |
| **文档** | 是否同步更新了相关文档？ |
| **每一行** | 人工编写的每行代码都要看。数据文件、生成代码可略过。 |
| **上下文** | 在更广阔上下文中审视——整个文件、整个系统。不接受降低代码健康状态的 CL。 |

## 审查流程

### 三步法浏览 CL

**第一步：整体视角**
- 阅读 CL 描述，理解变更做什么、为什么做。
- 这个变更本身是否合理？如果根本不应该做，立即回复说明原因并建议替代方案。

**第二步：审查核心部分**
- 找到变更量最大的"主"文件，先审查这些核心部分。
- 如果发现重大设计问题，立即发送评论——不等其他部分审查完。如果设计问题严重，大量后续代码可能被废弃。

**第三步：按合理顺序审查其余部分**
- 确认无重大设计问题后，按逻辑顺序浏览剩余文件。
- 有时先读测试代码再读主代码有助于理解变更意图。

## 审查标准详解

### 设计审查（最重要）

- CL 中各代码模块的交互是否合理？
- 变更应该放在当前代码库还是抽到库中？
- 是否与系统其余部分良好集成？
- 现在是添加此功能的合适时机吗？

### 功能审查

- CL 是否实现了开发者意图？意图对用户是否有利？
- 思考边界情况、并发问题、站在用户角度思考。
- 特别关注：**UI 变更**（难以仅靠读代码判断影响）、**并行编程**（可能导致死锁或竞态条件）。

### 复杂度审查

逐级检查：单行 → 函数 → 类是否过于复杂？

"过于复杂" = **代码读者无法快速理解**、**调用或修改时容易引入 bug**。

特别警惕**过度工程**：代码做得比需要的更通用，或添加了当前系统不需要的功能。解决**现在**已知的问题，而非猜测未来可能需要解决的问题。

### 测试审查

- 要求与变更匹配的单元测试、集成测试或端到端测试。测试应和产品代码在同一个 CL 中。
- 确保测试正确、合理、有用：
  - 代码损坏时测试会真的失败吗？
  - 底层代码变更时测试会误报吗？
  - 断言是否简单有用？
  - 不同测试方法之间是否合理分离？
- 测试也是需要维护的代码，不接受测试中的复杂度。

### 命名审查

好名字足够长以完全传达该项是什么/做什么，但不至于长到难以阅读。

### 注释审查

- 注释通常应解释**为什么**而非**是什么**。如果代码本身不够清晰，应该简化代码而非加注释。
- 例外：正则表达式和复杂算法通常需要注释解释它们在做什么。
- 查看 CL 之前的注释：是否有可移除的 TODO、是否有反对此变更的旧注释。
- 注释 ≠ 文档（类/模块/函数的文档应表达目的、用法和行为）。

### 风格审查

- 确保 CL 遵循相应风格指南。
- 想改进风格指南之外的内容，用 "Nit:" 前缀标注，表示非强制性。
- 不要仅基于个人风格偏好阻止提交。
- CL 不应将大规模风格变更与功能变更混在一起——格式化和功能应分开提交。

### 一致性

- 如果现有代码与风格指南不一致：风格指南是绝对权威，CL 应遵循指南。
- 如果风格指南是建议而非要求：倾向于遵循风格指南，除非局部不一致会造成更大混淆。

### 文档审查

- 如果 CL 改变了用户构建、测试、交互或发布代码的方式，检查是否同步更新了 README、文档页面和生成的参考文档。
- 如果 CL 删除或废弃代码，考虑文档是否也应删除。

## 评论写法

### 三项要点

1. **解释原因**——让开发者理解评论的意图、遵循的最佳实践、或建议如何改善代码健康。
2. **平衡直接指导和指出问题让开发者自己决定**——指出问题让开发者决策，但有时直接指令、建议甚至代码更有帮助。
3. **鼓励开发者简化代码或添加代码注释**，而非依赖于在审查工具中做解释。审查工具中的解释对未来代码读者没有帮助。

### 评论严重程度标签

| 标签 | 含义 |
|------|------|
| **Nit** | 小事，技术上应该做但影响不大 |
| **Optional / Consider** | 可能是个好主意，但不严格要求 |
| **FYI** | 不期望在本 CL 中修改，但未来可考虑 |

使用标签让审查意图明确，帮助作者确定优先级。

### 关于解释

如果要求开发者解释一段代码，通常结果应该是他们**重写代码使其更清晰**。偶尔添加代码注释也是合适的回应（前提是不是仅仅解释过于复杂的代码）。审查工具中写的解释对未来代码读者没有帮助。

## 处理"以后再清理"

开发者常表示会在后续 CL 中清理——经验表明，除非在**立即**写清理 CL，否则很少真正发生。

- 通常最好坚持开发者在代码进入代码库之前**现在**清理。"以后再清理"是代码库退化的常见方式。
- 如果 CL 引入了新的复杂度，必须在提交前清理（除非紧急情况）。
- 如果 CL 暴露了周围问题且现在无法解决，开发者应提交 bug 并分配给自己，可选在代码中写 TODO 引用该 bug。

## CL 指南

### 写好 CL 描述

CL 描述应传达：**做了什么变更？** + **为什么做这些变更？**

**第一行规则：** 简短摘要（祈使句），独立存在让未来搜索代码历史的人能快速理解。第一行后空一行。

**坏的描述：** "Fix bug." / "Fix build." / "Add patch." / "Phase 1."

**好的描述：**
```
RPC: 移除 RPC 服务器消息空闲列表的大小限制。

像 FizzBuzz 这样的服务器有非常大的消息，能从复用中受益。
增大空闲列表，并添加一个 goroutine 随时间缓慢释放空闲列表条目，
使空闲服务器最终释放所有空闲列表条目。
```

### 写小 CL

**一个 CL = 一个自包含的变更**，只解决一件事。通常 100 行合理，1000 行太大。文件分散程度也影响"大小"——一个文件中的 200 行可能 OK，散在 50 个文件中通常太大。

**拆分策略：**
- **基于堆叠**：写完一个小 CL 发出审查后，立即基于此 CL 写下一个。
- **按文件拆分**：需要不同评审者的文件分组，各自自包含。
- **水平拆分**：在技术栈各层之间创建共享代码或桩，隔离变更。
- **垂直拆分**：按功能拆分，每个功能是独立的全栈实现。
- **重构分离**：重构独立 CL，不与功能变更或 bug 修复混在一起。
- **测试在同一个 CL**：逻辑变更应包含新的或更新的测试。

## AI 审查执行指南

### 审查准备

```bash
git diff --cached
# 如果为空
git diff HEAD~1 HEAD
```

### 审查输出格式

```markdown
## 审查总结
[1-2 句整体评估]

## 设计层面
- [设计相关评论]

## 关键问题（必须修复）
- 问题 1：[描述 + 建议修复方案]
- 问题 2：...

## 建议（可选）
- 建议 1：[描述]

## 好的实践（肯定）
- [开发者做得好的地方]

## 问题
- [需要开发者澄清的问题]
```

### 常见问题速查

**Python**
```python
# 差: SQL 注入风险
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")
# 好: 参数化查询
cursor.execute("SELECT * FROM users WHERE id = ?", (user_id,))

# 差: Shell 注入
os.system(f"ls {user_input}")
# 好: 安全子进程
subprocess.run(["ls", user_input], check=True)

# 差: 危险 eval/exec
eval(user_input)
# 好: 使用安全的解析方式
```

**JavaScript**
```javascript
// 差: XSS 风险
element.innerHTML = userInput;
// 好: 安全文本内容
element.textContent = userInput;
```

### 需要特别关注的信号

- 硬编码的密钥、API Key、凭证
- 缺少输入验证
- SQL 查询使用字符串拼接
- 文件操作未验证路径（路径穿越风险）
- 外部调用缺少错误处理
- 遗留的调试 print/console.log
- 被注释掉的代码
- 新代码没有对应测试
- 过度工程（做了当前不需要的抽象/功能）
- 并发代码没有正确处理竞态条件
- 重构和功能变更混在同一个 CL 中
- 大规模格式化变更和逻辑变更混在一起

## Common Pitfalls

1. **只关注表面差异**——应审查变更的意图和设计，而非只是 diff 的语法正确性。
2. **跳过测试审查**——测试的质量和覆盖率同等重要，测试也是代码。
3. **接受"以后再清理"**——几乎不会发生。坚持现在处理。
4. **风格偏好冒充设计问题**——区分风格（遵循风格指南）和设计（基于原则权衡）。不确定时用 Nit 标签。
5. **忽略过度工程**——"以后可能需要"不是添加抽象的理由。解决现在的问题。
6. **大 CL 一把审查**——超过 1000 行应要求拆分，否则审查质量无法保证。
7. **功能和重构混在一起**——格式/重构变更应独立于功能变更，分开提交。

## Verification Checklist

- [ ] CL 描述清晰传达了做什么和为什么
- [ ] 审查了全部 10 个维度（设计、功能、复杂度、测试、命名、注释、风格、文档、每一行、上下文）
- [ ] 没有降低代码健康状态的变更被接受
- [ ] 复杂度和过度工程已评估
- [ ] 测试覆盖充分、测试设计合理
- [ ] 安全信号已检查（密钥、注入、路径穿越等）
- [ ] 评论带有严重程度标签（Nit/Optional/FYI）
- [ ] "以后再清理"已被拒绝（除非有立即的后续 CL）
- [ ] 输出格式遵循标准模板（审查总结 → 关键问题 → 建议 → 好的实践 → 问题）