code-review-and-quality

代码审查与质量

概述

多维度代码审查与质量门禁。每项变更在合并前都必须经过审查，无一例外。审查涵盖五个方面：正确性、可读性、架构、安全性和性能。

批准标准： 即使变更并非完美，只要它确实能改善整体代码健康度，就应批准。完美代码并不存在——我们的目标是持续改进。不要因为变更没有完全按照你会采用的方式编写，就阻止它。如果它改善了代码库并遵循项目规范，就批准它。

适用场景

• 合并任何 PR 或变更之前
• 完成某项功能实现之后
• 需要评估其他 Agent 或模型生成的代码时
• 重构现有代码时
• 修复任何 bug 之后（同时审查修复内容和回归测试）

五个方面的审查

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

1. 正确性

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

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

2. 可读性与简洁性

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

• 名称是否具有描述性，并符合项目规范？（没有上下文时，不要使用 temp、data、result）
• 控制流是否直观（避免嵌套三元表达式和深层回调）？
• 代码组织是否合理（相关代码归在一起，模块边界清晰）？
• 是否有应当简化的“巧妙”技巧？
• 能否用更少的代码行实现？（1000 行能完成的事，100 行就足够；否则就是失败）
• 抽象是否值得其带来的复杂性？（出现第三种用例之前，不要进行泛化）
• 添加注释是否有助于说明不明显的意图？（但不要注释显而易见的代码。）
• 是否存在死代码痕迹：无操作变量（_unused）、向后兼容垫片或 // removed 注释？
• 是否在无关的流程中硬塞了新的条件判断？ 这是设计异味，不是小问题——应将逻辑移入独立的辅助函数、状态或策略，而不是把现有路径搅在一起。
• 是否对相同结构重复出现条件判断？ 这说明缺少模型或分发器。“临时”分支通常会变成永久技术债务。

3. 架构

这项变更是否符合系统设计？

• 是否遵循现有模式，还是引入了新模式？如果是新模式，是否有充分理由？
• 是否保持了清晰的模块边界？
• 是否存在应当共享的重复代码？
• 依赖关系的流向是否正确（没有循环依赖）？
• 抽象层级是否恰当（不过度设计，也没有过度耦合）？
• 这次重构是在降低复杂性，还是只是在转移复杂性？ 数一数读者要理解这项变更必须记住多少个概念。如果所谓的“更整洁”版本并未减少这个数量，那它就不算更整洁——与其重新集中相同的逻辑，不如选择能让整个分支、模式或层级消失的重构。优先删除抽象，而不是打磨抽象。
• 特定功能的逻辑是否泄漏到共享模块或通用模块中？ 将逻辑放在其所属层中，复用现有的规范辅助函数，而不是创建近似的重复实现，也不要让架构偏移变得常态化。
• 类型边界是否明确？ 对多余的 any/unknown/可选类型/类型断言，以及掩盖不明确不变量的静默回退逻辑，都应提出质疑——明确边界通常能简化周围的控制流。

4. 安全性

如需了解详细的安全指南，请参阅 security-and-hardening。此更改是否引入了漏洞？

• 是否验证并清理了用户输入？
• 是否确保机密不会出现在代码、日志和版本控制中？
• 是否在需要的地方检查了身份验证／授权？
• SQL 查询是否使用参数化查询（不进行字符串拼接）？
• 是否对输出进行编码以防止 XSS？
• 依赖项是否来自可信来源，且不存在已知漏洞？
• 是否将来自外部来源的数据（API、日志、用户内容、配置文件）视为不可信数据？
• 是否在系统边界验证外部数据流，再将其用于逻辑处理或渲染？

5. 性能

如需了解详细的性能分析与优化指南，请参阅 performance-optimization。此更改是否引入了性能问题？

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

结构性改进方案

指出结构性问题时，请提出具体的改动方案，而不只是指出问题。只说“这很复杂”的评审会让作者无从下手。请提出明确的重构方式：

• 将条件判断链替换为类型化模型或显式分发器。
• 合并重复分支，形成更清晰的单一流程。
• 将编排与业务逻辑分离，让两者各自清晰易读。
• 将功能专属逻辑从共享模块移出，放入负责该概念的包中。
• 复用规范的辅助函数，而不是另写一个几乎相同的实现。
• 明确类型边界，避免下游分支判断。
• 删除只传递调用、却没有让 API 更清晰的封装层。
• 提取辅助函数，或将大型文件拆分为职责明确的模块。

优先选择能减少可变因素的方案，而不是将同样的复杂性扩散到各处。

更改规模

小而聚焦的更改更易于评审、更快合并，也更安全地部署。请以以下规模为目标：

~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.

关注文件大小，而不只是差异大小。 即使差异很小，也可能让文件超出合理范围——单个文件总行数约为 1000 行（不同于上文约 1000 行变更的阈值）是常见的审查提示，并非硬性上限。如果更改会显著增大一个已经很大的文件，请先考虑提取辅助函数、子组件或模块，再继续添加内容。先拆分，再添加。

“一项更改”的定义： 一项独立完整的修改，只解决一个问题，包含相关测试，并确保提交后系统仍可正常运行。它是一个功能的一部分，而不是整个功能。

更改规模过大时的拆分策略：

策略	做法	适用场景
堆叠	先提交一项小更改，再基于它开始下一项	存在顺序依赖
按文件组拆分	针对需要不同评审者的文件组分别提交更改	涉及多个领域
水平拆分	先创建共享代码／存根，再创建使用方	分层架构
垂直拆分	将功能拆解为更小的全栈切片	功能开发

允许较大更改的情况： 完整删除文件，以及只需验证意图、无需逐行检查的自动化重构。

将重构与功能开发分开。 同时重构现有代码并添加新行为的一项更改，实际上是两项更改——请分别提交。较小的清理工作（如重命名变量）可由评审者酌情决定是否一并提交。

变更描述

每项变更都需要一段独立完整的描述，以便留存在版本控制历史中。

首行： 简短、使用祈使语气且独立成句。例如，“删除 FizzBuzz RPC”，而不是“正在删除 FizzBuzz RPC”。描述要足够明确，让查阅历史记录的人无需阅读差异内容，也能理解变更。

正文： 说明变更内容及原因。补充代码本身无法体现的背景、决策和理由。适当时，链接到错误编号、基准测试结果或设计文档。若采用的方法存在不足，也应予以说明。

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

审查流程

第 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?

通过实验回答最后一个问题，而不是阅读代码。反转变更新增的一个条件（删除一个否定、将 && 换成 ||），运行测试套件，然后用副本还原文件。如果变异后测试仍然通过，就说明存在问题：指出缺失的测试用例。有关项目级变异得分，请参阅 constraint-driven-development。

第 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 步：对发现的问题分类

为每条评论标注严重程度，让作者了解哪些是必须处理的，哪些是可选的：

前缀	含义	作者应采取的行动
(无前缀)	必须变更	合并前必须处理
Critical：	阻止合并	安全漏洞、数据丢失、功能损坏
Nit：	轻微、可选	作者可以忽略——格式、风格偏好
Optional： / Consider：	建议	值得考虑，但不是必须的
FYI	仅供参考	无需采取行动——为将来提供背景信息

这样可以避免作者把所有反馈都当成强制要求，浪费时间处理可选建议。

优先指出重要问题。 按影响力排序发现的问题：先看正确性和安全性，再看结构性回归和错失的简化机会，最后看其他问题。不要让真正的问题被表面细节掩盖——几条有充分依据的评论胜过一长串意见。如果有一个结构性问题和十条细枝末节，那么结构性问题才是这次审查的重点。

第 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

这样可以发现单个模型可能遗漏的问题——不同模型各有盲点。

审查 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, Required, Optional, or Nit.

死代码清理

任何重构或实现变更后，都要检查是否有遗留的孤立代码：

1. 找出现在无法访问或未使用的代码
2. 明确列出这些代码
3. 删除前先询问：“是否应删除这些现在未使用的元素：[列表]？”

不要留下死代码——它会让未来的读者和 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?

审查速度

审查过慢会阻碍整个团队。切换上下文进行审查的成本，低于让他人等待的成本。

• 一个工作日内回复——这是最长时限，不是目标
• 理想节奏： 收到审查请求后尽快回复，除非你正专注于编码。典型的变更应在一天内完成多轮审查
• 优先快速逐次回复，而不是急于给出最终批准。即使需要多轮审查，及时反馈也能减少挫败感
• 大型变更： 请作者拆分变更，而不是一次审查一个庞大的变更集

处理分歧

解决评审争议时，请按以下优先级处理：

1. 技术事实和数据优先于意见和偏好
2. 风格指南是风格问题上的绝对准则
3. 软件设计必须依据工程原则评估，而非个人偏好
4. 代码库一致性可以接受，前提是不会损害整体健康状况

**不要接受“我之后会整理”。**经验表明，延后的整理很少会真正完成。除非确实遇到紧急情况，否则应要求在提交前完成整理。如果这次更改无法处理周边问题，则应要求提交 Bug，并由提交者自行认领。

评审中的诚信

评审代码时——无论代码是你、其他 Agent 还是人编写的：

• **不要敷衍通过。**没有评审依据的“LGTM”毫无帮助。
• **不要淡化实际问题。**如果某个问题会导致生产环境故障，却说“这可能只是个小问题”，就是不诚实。
• 尽可能量化问题。“这个 N+1 查询会让列表中的每个项目额外增加约 50 ms”比“这可能会很慢”更好。
• **对明显有问题的方案提出异议。**迎合是评审中的一种失职。如果实现存在问题，就直接指出并提出替代方案。
• **从容接受否决。**如果作者掌握完整背景且持不同意见，就尊重其判断。评论代码，而非针对个人——将个人批评改为聚焦代码本身。

依赖管理规范

代码评审也包括依赖评审：

添加任何依赖之前：

1. 现有技术栈能否解决这个问题？（通常可以。）
2. 这个依赖有多大？（检查对打包体积的影响。）
3. 是否仍在积极维护？（检查最近一次提交和未解决的问题。）
4. 是否存在已知漏洞？（npm audit）
5. 许可证是什么？（必须与项目兼容。）

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

升级现有依赖也是一项代码更改，而风险最高的升级，往往是那些成批合并、说明写着“bump deps”的更改。请按同样的规范进行评审：

1. **阅读变更日志，而不只是版本号。**语义化版本是维护者可能未遵守的承诺——“补丁”版本也可能带来行为变化。升级主版本时，请阅读迁移说明并查明可能出现的破坏性变更。
2. **每次更改只升级一个依赖。**逐个（或按小型相关组）升级并合并。批量升级导致构建失败时，你就无法确定是哪个包引起的；只更改一个包能让原因一目了然，也便于干净地回退。
3. **以测试结果为准。**升级前后测试套件都必须通过，才能验证升级；“安装成功”不算验证。如果对该依赖行为的测试覆盖较少，那么真正需要关注的就是这个缺口——先补上测试。
4. **留意传递依赖关系图。**大多数已安装的包都不是任何人直接选择的。请检查锁文件的差异，而不只是 package.json；一次直接依赖的升级就可能引入数十项间接更改。
5. **确保锁文件真实可靠。**提交锁文件，评审其差异，切勿手动编辑。锁文件才是真正锁定交付内容的文件。

对于如何分诊 npm audit 的发现以及供应链风险（仿冒拼写、维护者账户遭入侵），请遵循 security-and-hardening 技能——本节介绍的是升级工作流，该技能介绍的是安全判定。

评审检查清单

## 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
- [ ] Refactors reduce complexity rather than relocate it
- [ ] No feature logic in shared modules; file stays within a healthy size

### 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

另请参阅

• 有关详细的安全评审指南，请参阅 ../../references/security-checklist.md
• 有关性能评审检查，请参阅 ../../references/performance-checklist.md

常见自我开脱

自我开脱	事实
“能运行就够了”	难以阅读、不安全或架构错误的可运行代码会不断累积技术债。
“这是我写的，所以我知道它没问题”	作者往往意识不到自己的假设。每次改动都能从另一双眼睛的审视中获益。
“我们以后再整理”	以后永远不会到来。评审是质量关卡——善用它。要求在合并前完成整理，而不是合并后。
“AI 生成的代码应该没问题”	AI 代码需要更严格的审查，而不是更少。它看起来很有把握、很可信，即使实际上是错的。
“测试通过了，所以代码没问题”	测试是必要条件，但不是充分条件。它们无法发现架构问题、安全问题或可读性问题。
“重构让代码更整洁了”	转移复杂性并不等于减少复杂性。如果读者仍需掌握同样多的概念，结构就没有改善——应寻找能让分支消失的方案。
“只是在这个文件里加一点小改动”	即使差异很小，也可能让文件超过健康的规模，还可能把分支硬塞进无关的流程。应评判改动后的结构，而不是差异大小。
“只是升级版本”	升级会带来你没有编写的行为变更。请阅读变更日志；语义化版本并不保证没有破坏性变更。
“我会在一个 PR 里升级所有内容，节省时间”	批量升级导致构建失败时，很难看出是哪个包引起的。每次改动只升级一个依赖，才能让原因明确，也便于干净地回滚。

危险信号

• PR 未经任何评审就合并
• 评审只检查测试是否通过（忽视其他方面）
• 没有实际评审证据就说“LGTM”
• 涉及安全的改动未经侧重安全的评审
• 过大的 PR “大到无法认真评审”（应拆分）
• 修复 Bug 的 PR 没有回归测试
• 评审意见没有严重程度标签——无法明确哪些是必须修改的，哪些是可选的
• 接受“我以后会修”——这种事永远不会发生
• 重构只是挪动代码，却没有减少读者必须掌握的概念数量
• 已经很大的文件继续增长，而不是拆分
• 在无关的代码路径中散布新的条件分支（缺少抽象）
• 定制辅助函数重复实现了现有的标准辅助函数，或将功能逻辑放进共享模块
• 批量“升级依赖” PR 未审查变更日志，也未按包隔离
• 锁文件由手动编辑、未提交，或合并前未审查其差异

验证

评审完成后：

• 所有 Critical 问题均已解决
• 所有 Required（无前缀）改动均已解决，或已明确延期并说明理由
• 测试通过
• 构建成功
• 已记录验证过程（改了什么、如何验证）
• 已根据变更日志审查依赖升级，按包隔离，并通过全绿的测试套件验证，且已审查锁文件差异

推定为阻塞项： 对以下每项指出问题并提出更简单的设计；只有在改动确实使结构变差时，才升级为 Required：重构只是转移复杂性而非减少复杂性；改动使文件超过规模边界，却没有拆分；将功能逻辑添加到共享模块；与现有标准辅助函数高度重复；用静默回退掩盖不明确的不变量。