233i 代码评审与质量
概览
带质量门禁的多维度代码评审。每一项改动在合并前都必须经过评审，没有例外。评审覆盖五个维度：正确性、可读性、架构、安全性和性能。
批准标准： 当一个改动能够明确提升整体代码健康度时，就应该批准，即使它还不完美。完美代码不存在，目标是持续改进。不要因为“不是你会写出来的样子”就阻塞变更。只要它让代码库变得更好，并且遵守项目约定，就应该批准。
何时使用
- 合并任何 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.
死代码卫生
任何重构或实现完成后，都检查一下有没有遗留孤儿代码：
- 找出现在不可达或未使用的代码
- 明确列出来
- 删除前先问： “这些现在未使用的元素是否要一并删除：[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 里出现争议时，按以下优先级处理：
- 技术事实和数据 高于个人意见和偏好
- Style guide 是风格问题的最高裁决
- 软件设计 应按工程原则评估，而不是按个人口味
- 代码库一致性 只要不损害整体健康，就可以接受
不要接受“我之后再清理”。 经验表明，延期清理几乎不会真的发生。除非是真正的紧急情况，否则应要求在提交前清理完。如果本次改动确实无法顺手解决周边问题，也至少要求创建 bug 并由作者自我指派。
评审中的诚实
无论你评审的是自己、其他 agent 还是人类写的代码：
- 不要橡皮图章式通过。 没有证据的 “LGTM” 对谁都没帮助。
- 不要弱化真实问题。 明明是会打到生产上的 bug，却说成“可能有一点点小问题”，这是不诚实。
- 能量化就量化。 “这个 N+1 查询会让列表里每个元素多出约 50ms” 比 “这可能会慢” 更有价值。
- 面对明显有问题的做法要敢于指出。 在 review 中一味迎合也是失败模式。如果实现有问题，就直接说明，并给替代方案。
- 接受 override。 如果作者掌握完整上下文且不同意你的判断，可以接受。评论代码，不评论人，把人身评价重写为对代码本身的评价。
依赖纪律
代码评审也包括依赖评审：
在添加任何依赖前：
- 现有技术栈能不能解决？很多时候其实可以
- 这个依赖有多大？会不会影响 bundle
- 它是否仍在活跃维护？看最近提交和 issue
- 有没有已知漏洞？跑
npm audit - 许可证是否兼容项目？
规则： 优先使用标准库和现有工具，而不是新依赖。每一个依赖都是一项负债。
评审清单
## 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 问题要么已解决，要么有明确理由延期
- 测试通过
- 构建成功
- 验证故事已经记录清楚，包括改了什么、如何验证