code-review-and-quality

代码审查与质量

概述

多维代码审查，带质量门禁。审查覆盖五个维度：正确性、可读性、架构、安全性、性能。

何时使用

• 合并任何 PR 或变更之前
• 完成功能实现之后
• 当另一个 Agent 或模型产出的代码需要评估时
• 重构现有代码时
• 任何 bug 修复之后（审查修复本身和回归测试）

五轴审查

1. 正确性

代码是否做到了它声称要做的事？

• 是否匹配 spec 或任务需求？
• 边界情况是否处理（null、空值、边界值）？
• 错误路径是否处理（不仅 happy path）？
• 是否通过所有测试？测试是否确实在测试正确的东西？
• 是否有差一错误、竞态条件或状态不一致？

2. 可读性与简洁性

另一个工程师（或 Agent）能在没有作者解释的情况下理解这段代码吗？

• 命名是否描述性且与项目约定一致？（没有上下文时不用 temp、data、result）
• 控制流是否直观（避免嵌套三元、深层回调）？
• 代码是否逻辑有序（相关代码分组、清晰的模块边界）？
• 是否有应该简化的"聪明"技巧？
• 能用更少的行数完成吗？（100 行就够了却写了 1000 行就是失败）
• 抽象是否物有所值？（在第三个用例之前不要泛化）
• 添加注释是否能澄清非显而易见的意图？（但不要注释显而易见的代码。）
• 是否有死代码残留：空操作变量、向后兼容 shim、或 // removed 注释？

3. 架构

变更是否适合系统设计？

• 是遵循已有模式还是引入新模式？如果是新的，是否有充分理由？
• 是否保持模块边界干净？
• 是否有应该共享的代码重复？
• 依赖方向是否正确（无循环依赖）？
• 抽象层级是否合适（不过度工程、不太耦合）？

4. 安全性

详细安全指导见 security-and-hardening。变更是否引入漏洞？

• 用户输入是否验证和清理？
• 密钥是否不暴露在代码、日志和版本控制中？
• 认证/授权是否在需要的地方检查？
• 查询是否参数化（无字符串拼接）？
• 输出是否编码以防止注入攻击？
• 依赖是否来自可信来源且无已知漏洞？
• 来自外部来源的数据（API、日志、用户内容、配置文件）是否被当作不信任数据处理？

5. 性能

变更是否引入性能问题？

• 是否存在 N+1 查询模式？
• 是否存在无界循环或无约束的数据抓取？
• 是否存在应该是异步的同步操作？
• 列表端点是否缺少分页？
• 热路径中是否创建了大对象？

变更大小

小、聚焦的变更更容易审查、更快速合并、更安全部署。以这些大小为目标：

~100 行变更    → 优秀。可以一次性审查完毕。
~300 行变更    → 可接受，如果是单个逻辑变更。
~1000 行变更   → 太大了。拆分它。

"一个变更"意味着什么： 一个单独的自包含修改，处理一件事，包含相关测试，提交后系统保持在功能状态。功能的一部分——不是整个功能。

变更太大时的拆分策略：

策略	方法	适用场景
堆叠	提交一个小变更，基于它开始下一个	串行依赖
按文件组	对需要不同审查者的人员组分出独立变更	跨领域关注
横向	先创建共享代码/存根，再创建消费者	分层架构
纵向	拆分为功能的更小全栈切片	功能开发

大变更可接受的情况： 完整文件删除和自动化重构——审查者只需验证意图，而非每一行。

将重构与功能分开。 一个既重构已有代码又添加新行为的变更是两个变更——分别提交。小清理（变量重命名）可在审查者酌情下包含。

变更描述

每个变更需要一个在版本控制历史中独立存在的描述。

前缀: feat、fix、refactor、docs、test、chore、perf、style 等。选择最能描述变更类型的前缀。

第一行： 简短、祈使语气、独立。"删除 FizzBuzz RPC"而非"正在删除 FizzBuzz RPC"。必须信息足够，使搜索历史的人无需阅读 diff 就能理解变更。

正文： 什么在变，为什么。包含代码本身不可见的上下文、决策和推理。链接到 bug 编号、基准测试结果或设计文档（如果有）。当方法存在不足时承认它。

反模式： "修复 bug"、"修复构建"、"添加补丁"、"把代码从 A 移到 B"、"阶段 1"、"添加便利功能"。

审查流程

第 1 步：理解上下文

在看代码之前，理解意图：

- 这个变更试图达到什么目的？
- 它实现了什么 spec 或任务？
- 预期的行为变更是什么？

第 2 步：先审查测试

测试揭示意图和覆盖：

- 是否存在针对此变更的测试？
- 它们是否测试行为（而非实现细节）？
- 边界情况是否覆盖？
- 测试是否有描述性名称？
- 如果代码改变了，测试能否捕获回归？

第 3 步：审查实现

以五轴视角浏览代码：

对每个变更的文件：
1. 正确性：这段代码做到了测试说它应该做的事吗？
2. 可读性：我能不需帮助就理解它吗？
3. 架构：这适合系统设计吗？
4. 安全性：有漏洞吗？
5. 性能：有瓶颈吗？

第 4 步：分类发现

为每一条评论标注严重级别，使作者知道什么是必须的、什么是可选的：

前缀	含义	作者行为
(无前缀)	必须的变更	合并前必须处理
Critical:	阻止合并	安全漏洞、数据丢失、损坏的功能
Nit:	次要，可选	作者可忽略——格式、风格偏好
Optional: / Consider:	建议	值得考虑但不必须
FYI	仅供参考	无需行动——为未来参考的上下文

第 5 步：验证过程

检查作者的验证叙述：

- 运行了什么测试？
- 构建通过了吗？
- 变更有手动测试过吗？
- UI 变更有截图吗？
- 有前后对比吗？

审查检查清单

## Review: [PR/变更标题]

### 上下文
- [ ] 我理解这个变更是做什么的，为什么

### 正确性
- [ ] 变更匹配 spec/任务需求
- [ ] 边界情况已处理
- [ ] 错误路径已处理
- [ ] 测试充分覆盖变更

### 可读性
- [ ] 名称清晰且一致
- [ ] 逻辑直观
- [ ] 无不必要复杂度

### 架构
- [ ] 遵循已有模式
- [ ] 无不必要耦合或依赖
- [ ] 合适的抽象层级

### 安全性
- [ ] 代码中无密钥
- [ ] 输入在边界验证
- [ ] 无注入漏洞
- [ ] 认证检查到位
- [ ] 外部数据源被视为不信任数据

### 性能
- [ ] 无 N+1 模式
- [ ] 无无界操作
- [ ] 列表端点有分页

### 验证
- [ ] 测试通过
- [ ] 构建成功
- [ ] 手动验证完成（如适用）

### 裁决
- [ ] **批准** — 准备好合并
- [ ] **请求修改** — 问题必须处理

参考

• 详细安全审查指导见 references/security-checklist.md

警告信号

• PR 在没有任何审查的情况下被合并
• 审查只检查测试是否通过，忽略其他维度
• 安全敏感变更没有安全聚焦审查
• 大 PR 太大无法适当审查（拆分它们）
• Bug 修复 PR 没有回归测试

验证

审查完成后：

• 所有 Critical 问题已解决
• 所有 Important 问题已解决或明确延期且有正当理由
• 测试通过
• 构建成功
• 验证叙述已记录（什么变更了，如何验证的）