code-review-and-quality

代码审查与质量

概述

带质量门禁的多维度代码审查。每次变更在合并前都必须经过审查——没有例外。审查涵盖五个维度：正确性、可读性、架构、安全性和性能。

批准标准： 当变更确实能改善整体代码健康度时批准，即使它并不完美。完美的代码不存在——目标是持续改进。不要因为代码不是你写的方式就阻止变更。如果它改进了代码库并遵循项目约定，就批准它。

何时使用

• 在合并任何 PR 或变更之前
• 完成功能实现后
• 当另一个智能体或模型生成了你需要评估的代码时
• 重构现有代码时
• 在修复任何 bug 之后（同时审查修复和回归测试）

五维度审查

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

1. 正确性

代码是否实现了它声称的功能？

• 是否符合规范或任务要求？
• 是否处理了边界情况（null、空值、边界值）？
• 是否处理了错误路径（不仅仅是正常路径）？
• 是否通过了所有测试？测试是否真正测试了正确的东西？
• 是否存在差一错误、竞态条件或状态不一致？

2. 可读性与简洁性

其他工程师（或智能体）能否在不需作者解释的情况下理解这段代码？

• 命名是否描述性强且与项目约定一致？（避免无上下文的 temp、data、result）
• 控制流是否直接明了（避免嵌套三元表达式、深层回调）？
• 代码组织是否逻辑清晰（相关代码分组、模块边界清晰）？
• 是否存在应简化的“巧妙”技巧？
• 能否用更少的行数实现？（1000 行而 100 行足够就是失败）
• 抽象是否值得其复杂性？（不要过早泛化，直到第三次使用场景）
• 注释是否有助于澄清非显而易见的意图？（但不要注释显而易见的代码。）
• 是否存在死代码：无操作变量（_unused）、向后兼容的 shim 或 // removed 注释？
• 是否将新的条件分支硬塞到不相关的流程中？ 这是设计异味，不是小问题——将逻辑推入自己的辅助函数、状态或策略中，而不是纠缠现有路径。
• 是否出现对同一形状的重复条件判断？ 这暗示缺少模型或分发器。“临时”分支通常是永久债务。

3. 架构

变更是否符合系统设计？

• 是否遵循现有模式或引入了新模式？如果是新的，是否有合理理由？
• 是否保持了清晰的模块边界？
• 是否存在应共享的代码重复？
• 依赖方向是否正确（无循环依赖）？
• 抽象层次是否合适（不过度设计，不过度耦合）？
• 这次重构是降低了复杂性还是仅仅转移了复杂性？ 计算读者需要掌握的概念数量以理解变更。如果“更干净”的版本没有改变这个数量，那它并不更干净——优先选择能让整个分支、模式或层消失的重构，而不是重新集中相同逻辑。优先删除抽象而不是打磨它。
• 功能特定逻辑是否泄漏到共享或通用模块中？ 将逻辑保留在其所属层，重用现有的规范辅助函数而不是近似的副本，不要使架构漂移正常化。
• 类型边界是否明确？ 质疑随意的 any/unknown/可选/类型转换以及掩盖不明确不变量的静默回退——明确边界通常会使周围的控制流更简单。

4. 安全性

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

• 用户输入是否经过验证和清理？
• 密钥是否未出现在代码、日志和版本控制中？
• 是否在需要的地方检查了身份验证/授权？
• SQL 查询是否参数化（无字符串拼接）？
• 输出是否经过编码以防止 XSS？
• 依赖是否来自可信来源且无已知漏洞？
• 来自外部来源（API、日志、用户内容、配置文件）的数据是否被视为不可信？
• 外部数据流是否在系统边界处经过验证，然后才用于逻辑或渲染？

5. 性能

详细性能分析和优化请参见 performance-optimization。变更是否引入性能问题？

• 是否存在 N+1 查询模式？
• 是否存在无界循环或不受限制的数据获取？
• 是否存在应异步的同步操作？
• UI 组件中是否存在不必要的重新渲染？
• 列表端点是否缺少分页？
• 热路径中是否创建了大对象？

结构性修复方案

当你指出结构性问题时，不仅要指出问题，还要提出移动方案。只说“这很复杂”的审查会让作者猜测。采用命名的重构方式：

• 用类型化模型或显式分发器替换条件链
• 将重复分支合并为单一更清晰的流程
• 将编排与业务逻辑分离，使各自独立可读
• 将功能特定逻辑移出共享模块，放入拥有该概念的包中
• 重用规范辅助函数而不是定制的近似副本
• 明确类型边界，使下游分支消失
• 删除透传包装器，它增加了间接性但没有澄清 API
• 提取辅助函数，或将大文件拆分为专注的模块

优先选择能减少移动部件的修复方案，而不是将相同复杂性扩散开来的方案。

变更规模

小而专注的变更更容易审查、更快合并、更安全部署。目标规模如下：

~100 行变更   → 良好。一次审查即可完成。
~300 行变更   → 可接受，如果是单一逻辑变更。
~1000 行变更  → 太大。请拆分。

关注文件大小，而不仅仅是差异大小。 小的差异仍可能将文件推过健康边界——单个文件总行数约 1000 行（区别于上述 ~1000 行变更阈值）是常见的检查信号，不是硬性上限。当变更显著增大已很大的文件时，询问是否应先提取辅助函数、子组件或模块，然后再添加更多内容。先分解，再添加。

什么是“一个变更”： 一个自包含的修改，解决一个问题，包含相关测试，并在提交后保持系统功能正常。功能的一部分——而不是整个功能。

变更过大时的拆分策略：

策略	方式	适用场景
堆叠	提交一个小变更，基于它开始下一个	顺序依赖
按文件组	为需要不同审查者的组分别变更	横切关注点
水平	先创建共享代码/桩，然后是消费者	分层架构
垂直	将功能拆分为更小的全栈切片	功能开发

何时大变更可接受： 完整文件删除和自动化重构，审查者只需验证意图，无需逐行检查。

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

变更描述

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

第一行： 简短、祈使句、独立。写“Delete the FizzBuzz RPC”而不是“Deleting the FizzBuzz RPC。”必须信息充分，使搜索历史的人无需阅读差异就能理解变更。

正文： 变更了什么以及为什么。包含代码中不可见的上下文、决策和理由。必要时链接到 bug 编号、基准测试结果或设计文档。承认方法的缺点（如果存在）。

反模式： “修复 bug”、“修复构建”、“添加补丁”、“将代码从 A 移到 B”、“阶段 1”、“添加便利函数”。

审查流程

步骤 1：理解上下文

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

- 这个变更试图完成什么？
- 它实现了什么规范或任务？
- 预期的行为变化是什么？

步骤 2：先审查测试

测试揭示意图和覆盖率：

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

步骤 3：审查实现

带着五个维度遍历代码：

对于每个变更的文件：
1. 正确性：这段代码是否做了测试声称的事？
2. 可读性：我能否无需帮助理解它？
3. 架构：它是否适合系统？
4. 安全性：是否存在漏洞？
5. 性能：是否存在瓶颈？

步骤 4：分类发现

为每条评论标记严重性，以便作者知道哪些是必需的，哪些是可选的：

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

这防止作者将所有反馈视为强制性的，并在可选建议上浪费时间。

优先处理重要事项。 按影响力排序发现：正确性和安全性优先，然后是结构性回归和遗漏的简化，最后是其他所有内容。不要将真正的问题埋没在表面的小问题下——几条高确信度的评论胜过一长串。如果你有一个结构性问题和十个表面问题，结构性问题是审查的重点。

步骤 5：验证验证

检查作者的验证说明：

- 运行了哪些测试？
- 构建是否通过？
- 是否手动测试了变更？
- UI 变更是否有截图？
- 是否有前后对比？

多模型审查模式

使用不同模型进行不同角度的审查：

模型 A 编写代码
    │
    ▼
模型 B 审查正确性和架构
    │
    ▼
模型 A 处理反馈
    │
    ▼
人类做出最终决定

这能捕获单个模型可能遗漏的问题——不同模型有不同的盲点。

审查智能体的示例提示：

审查此代码变更的正确性、安全性和是否符合项目约定。
规范说 [X]。变更应该 [Y]。
将问题标记为 Critical、Required、Optional 或 Nit。

死代码清理

在任何重构或实现变更后，检查孤立代码：

1. 识别现在不可达或未使用的代码
2. 明确列出它们
3. 删除前询问： “我应该删除这些现在无用的元素吗：[列表]？”

不要留下死代码——它会混淆未来的读者和智能体。但也不要静默删除你不确定的东西。如有疑问，请询问。

识别的死代码：
- src/utils/date.ts 中的 formatLegacyDate() —— 已被 formatDate() 替代
- src/components/ 中的 OldTaskCard 组件 —— 已被 TaskCard 替代
- src/config.ts 中的 LEGACY_API_URL 常量 —— 无剩余引用
→ 可以安全删除这些吗？

审查速度

缓慢的审查会阻塞整个团队。上下文切换到审查的成本小于强加给他人的等待成本。

• 在一个工作日内响应——这是最大值，不是目标
• 理想节奏： 审查请求到达后不久即响应，除非正在深度专注编码。一个典型的变更应在一天内完成多轮审查
• 优先快速个人响应而非快速最终批准。即使需要多轮，快速反馈也能减少挫败感
• 大变更： 要求作者拆分，而不是审查一个巨大的变更集

处理分歧

解决审查争议时，应用此层级：

1. 技术事实和数据优先于意见和偏好
2. 风格指南是风格问题的绝对权威
3. 软件设计必须基于工程原则评估，而非个人偏好
4. 代码库一致性可接受，只要不降低整体健康度

不要接受“我稍后清理。” 经验表明推迟的清理很少发生。要求提交前清理，除非是真正的紧急情况。如果周围问题无法在此变更中解决，要求提交 bug 并自我分配。

审查中的诚实

审查代码时——无论是由你、其他智能体还是人类编写：

• 不要走过场。 没有审查证据的“LGTM”对任何人都没有帮助。
• 不要软化真正的问题。 当这是一个会影响生产的 bug 时，说“这可能是个小问题”是不诚实的。
• 尽可能量化问题。 “这个 N+1 查询将为列表中的每个项目增加约 50ms”比“这可能很慢”更好。
• 对有明确问题的方法提出反对。 阿谀奉承是审查中的失败模式。如果实现有问题，直接说出来并提出替代方案。
• 优雅地接受否决。 如果作者有完整上下文并不同意，尊重他们的判断。评论代码而非人——将个人批评重新聚焦到代码本身。

依赖纪律

代码审查的一部分是依赖审查：

在添加任何依赖之前：

1. 现有技术栈是否解决了这个问题？（通常是的。）
2. 依赖有多大？（检查对打包大小的影响。）
3. 它是否活跃维护？（检查最后提交、开放问题。）
4. 是否有已知漏洞？（npm audit）
5. 许可证是什么？（必须与项目兼容。）

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

审查清单

## 审查：[PR/变更标题]

### 上下文
- [ ] 我理解这个变更做了什么以及为什么

### 正确性
- [ ] 变更符合规范/任务要求
- [ ] 边界情况已处理
- [ ] 错误路径已处理
- [ ] 测试充分覆盖了变更

### 可读性
- [ ] 命名清晰且一致
- [ ] 逻辑直接明了
- [ ] 没有不必要的复杂性

### 架构
- [ ] 遵循现有模式
- [ ] 没有不必要的耦合或依赖
- [ ] 抽象层次适当
- [ ] 重构降低了复杂性而非转移了复杂性
- [ ] 共享模块中没有功能逻辑；文件保持在健康大小内

### 安全性
- [ ] 代码中没有密钥
- [ ] 在边界处验证了输入
- [ ] 没有注入漏洞
- [ ] 身份验证检查到位
- [ ] 外部数据源被视为不可信

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

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

### 裁决
- [ ] **批准**——准备合并
- [ ] **请求变更**——问题必须解决

参见

• 详细安全审查指南，请参见 references/security-checklist.md
• 性能审查检查项，请参见 references/performance-checklist.md

常见合理化借口

合理化	现实
“它能工作，这就够了”	能工作但不可读、不安全或架构错误的代码会累积债务。
“我写的，所以我知道它是正确的”	作者对自己的假设视而不见。每次变更都受益于另一双眼睛。
“我们稍后清理”	稍后永远不会来。审查就是质量门禁——使用它。要求合并前清理，而不是之后。
“AI 生成的代码可能没问题”	AI 代码需要更多审查，而不是更少。它自信且看似合理，即使出错时也是如此。
“测试通过了，所以没问题”	测试是必要的但不充分。它们不能捕获架构问题、安全问题或可读性问题。
“重构使它更干净”	转移复杂性不是降低复杂性。如果读者仍然需要掌握相同数量的概念，结构没有改进——寻找分支消失的版本。
“只是对这个文件的小添加”	小差异仍会将文件推过健康大小，并将分支硬塞到不相关的流程中。判断结果结构，而不是差异大小。

红旗

• PR 未经任何审查就合并
• 只检查测试是否通过的审查（忽略其他维度）
• 没有实际审查证据的“LGTM”
• 安全敏感变更没有安全重点审查
• 大 PR “太大无法正确审查”（拆分它们）
• bug 修复 PR 没有回归测试
• 审查评论没有严重性标签——不清楚哪些是必需的，哪些是可选的
• 接受“我稍后修复”——这永远不会发生
• 重构只是移动代码而没有减少读者需要掌握的概念数量
• 变更使已很大的文件变大而不是分解它
• 新的条件分支散布到不相关的代码路径中（缺少抽象）
• 定制的辅助函数重复了现有的规范函数，或功能逻辑放在共享模块中

验证

审查完成后：

• [ ] 所有 Critical 问题已解决
• [ ] 所有 Required（无前缀）变更已解决或明确推迟并附有理由
• [ ] 测试通过
• [ ] 构建成功
• [ ] 验证说明已记录（变更了什么，如何验证的）

推定阻塞项： 针对以下情况提出更简单的设计；仅在变更主动使结构恶化时升级为 Required：重构转移了复杂性而非降低它；变更将文件推过大小边界且未分解；功能逻辑添加到共享模块；现有规范辅助函数的近似副本；隐藏不明确不变量的静默回退。