Code Review

---
name: code-review
description: 审查代码（自己的或别人的），需有重点、分层次地看时。
---

# Code Review

## 概述

对代码做**有重点、分层次**的审查——先抓正确性和安全问题，再管可读性，风格交给工具。核心：好的 review 让作者知道"哪里有真问题、该先改什么"；坏的 review 是一堆 nit 淹没重点，或一句"看起来不错"放过错误。

## 何时使用

- 提交前审查自己或别人的代码（自我 review 见下文专门技巧）
- 同事让你看他的 PR
- 不确定 review 该看什么、怎么反馈

**不该用**：纯风格/格式问题（交给 linter/formatter 自动化，不该占用人 review）；想确认"功能对不对"——那是 verify-and-fix（实际跑）的事，review 是静态审查、替代不了运行验证。

**与相邻 skill 的边界**：`code-review` 是**审查视角**（找问题、分严重度、给建议），`verify-and-fix` 是**修复视角**（改对、验证）。code-review 发现的问题，需要修的进入 verify-and-fix 流程；review 中发现的"行为不对/报错"可能要先用 `debugging` 定位根因。三者接力：review 找问题 → debugging 定位 → verify-and-fix 修+验证。

## 核心内容

### 正确性优先，别被风格带偏

review 最常见的错是**逐行挑风格**（命名、缩进、注释多少）而放过**正确性**（逻辑对不对、边界处理了吗、有没有竞态）。正确性问题会让程序出错，风格问题只是不好看——前者 critical，后者 nit，优先级天差地别。

按重要性排序，review 该查的维度：

1. **正确性**：逻辑对吗？能跑通预期路径吗？有没有逻辑漏洞？（最该花时间）
2. **边界与异常**：空值/空集合/零/负数/超大输入怎么处理？外部依赖失败（网络/DB）呢？
3. **安全**：有没有注入（SQL/命令/XSS）？密码/密钥处理对吗？权限检查到位吗？敏感信息泄露吗？
4. **并发**：共享状态有竞态吗？异步顺序依赖对吗？资源泄漏（未关闭的连接/锁未释放）？
5. **可维护性**：命名清不清楚？结构是否过度复杂？有没有重复？（这里才是 nit 的领地）
6. **测试**：有测试吗？测的是行为还是实现？覆盖了关键路径和边界吗？

前 4 类是"会让程序出错或出事"的，必须查；第 5 类是"让人难受"的，次要；风格细节（缩进/格式）不该人查，交给工具。

**按改动性质调整重点**：上面是通用清单，但不同代码该重点查的不同——别对所有代码平均用力。涉及**钱/库存/计数**的，重点查原子性和一致性（中途失败会不会凭空产生/消失）；涉及**外部输入**的（用户输入、API、文件），重点查安全（注入、越权）；涉及**共享状态/异步**的，重点查竞态和资源泄漏；涉及**配置/迁移**的，重点查回滚和兼容。先识别"这段代码的风险面在哪"，把 review 力量集中投到那里。

### 分严重度，别把 nit 和 critical 混着

review 反馈必须**分严重度**，让作者知道先改什么：

- **阻塞（blocking / critical）**：必须改才能合并——逻辑错、安全漏洞、会崩溃、数据丢失风险。
- **重要（important）**：强烈建议改——边界没处理、缺少测试、设计有隐患，但不阻塞本次合并。
- **建议（nit / suggestion）**：可选——命名、可读性、小重构。改了更好，不改也能过。

不分层的 review 有两种失败：把 nit 当 critical（作者被一堆小事压垮，反而漏改真问题）、把 critical 当 nit（真问题被淹没在风格意见里）。**nit 要克制**——堆 15 条 nit 是在浪费作者时间，把同类 nit 归并成一条"风格建议"，或直接交给 linter。

### 反馈要带依据和建议，不只是"这不好"

每条 review 意见应该让作者能**理解问题 + 知道怎么改**：

- **指出问题 + 为什么是问题**：不说"这写得不好"，说"`user.name` 在 user 为 null 时会抛 TypeError——db.find 找不到时返回 null"。
- **给方向或示例**：不只说"改一下"，给"加个 null 检查，找不到时抛业务错误或返回 null，看调用方期望"。
- **区分事实和偏好**："这里有 null 风险"（事实，基于代码）vs "我觉得该用 early return"（偏好，标注是建议）。

带依据的反馈让作者能判断对错、学到东西；空泛的"不好"让作者只能盲从或抵触。

### 别只看 diff，要看整体

review 容易陷入"逐行看 diff"，但很多问题在**整体层面**才看得出来：

- 这个改动**和系统的其他部分一致吗**（命名约定、错误处理风格、架构分层）？
- 改动**有没有破坏既有契约**（改了函数签名，调用方都更新了吗）？
- **缺了什么**（diff 里只有 happy path，错误处理呢？只有功能代码，测试呢？）？

diff 告诉你"改了什么"，但要结合"整体该怎么"才能看出"改对了吗、改全了吗"。

### 大 PR：分块 + 抓主线，别试图一口气读完

大改动（几百行、多文件）的 review 容易陷入"看不完、看到后面忘前面"。策略：

- **先读 PR 描述/commit message**：作者说这次改了什么、为什么——这是主线，review 时随时对照"改动是否服务这个主线、有没有跑题"。
- **按逻辑分块，不按文件**：把改动按"功能模块"分组（如"认证部分""数据迁移""UI"），一块一块审，每块审完总结"这块改了啥、有没有问题"，再下一块。
- **抓主线正确性，细节分层**：先确认主线逻辑通（核心路径对不对），再下沉到边界/异常。主线错了一切白搭；主线对了再逐块找细节问题。
- **太大就要求拆**：如果一个 PR 改了互不相关的多件事，要求作者拆成几个 PR——大而杂的 PR 几乎无法有效 review，这不是 reviewer 的问题，是 PR 的问题。

### 自我 review：换视角，先跑后看

提交前自查自己的代码，有两个反直觉但有效的技巧：

- **放一会儿再看 / 换视角**：刚写完时代码在大脑的"短期记忆"里，你会自动补全没写好的地方、跳过自己的盲点。放几小时（或睡一觉）再看，或假装在 review 别人的代码——视角一换，自己的问题就显形了。
- **先用工具跑，再用人眼看**：先跑测试/lint/类型检查，让工具抓住机械问题（编译错、类型错、明显的 lint）；人眼集中查工具抓不到的——逻辑对不对、边界处理了吗、命名清不清楚。工具和人各查擅长的，别让人干工具的活。
- **对照 commit message 自查**：你这次提交说"修了 X"，那 diff 里应该只有修 X 相关的改动——混进去的无关改动（顺手重构、调试代码）会被这个对照揪出来。

自我 review 的产出标准同对外 review：**说出查了什么维度、发现什么**，而不是"我看过了没问题"。

## 常见错误

| 问题 | 修法 |
|------|------|
| 表面附和"看起来不错" | 列出实际查了哪些维度、发现什么，空 review 等于没 review |
| 逐行挑 nit 淹没正确性 | 正确性/安全/边界优先，nit 克制并归并 |
| 不分严重度，nit 和 critical 混着 | 分阻塞/重要/建议三层，让作者知道先改什么 |
| 只说"不好"不说怎么改 | 每条带"为什么是问题 + 改的方向/示例" |
| 只看 diff 逐行 | 结合整体：一致性、契约破坏、缺了什么 |
| 风格问题占用人 review | 交给 linter/formatter，人查判断性问题 |
| 只看功能不看测试 | 查测试是否存在、测的是行为还是实现、覆盖关键路径 |
| 大 PR 一口气读，读到后面忘前面 | 先读描述抓主线，按逻辑分块审，太大就要求拆 PR |
| 自我 review 刚写完就看 | 放一会儿/换视角，先用工具跑再人眼看，对照 commit message 查混入的无关改动 |