code-review-and-quality

name	code-review-and-quality
description	进行多维度代码评审。适用于合并任何变更之前。适用于评审由你自己、另一个 agent 或人类编写的代码。适用于在代码进入主分支之前，需要跨多个维度评估代码质量的场景。

代码评审与质量

概述

带质量门控的多维代码评审。每个变更在合并之前都需经过评审——无例外。评审覆盖五个维度：正确性、可读性、架构、安全性和性能。

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

适用场景

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

五维度评审

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

1. 正确性

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

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

2. 可读性与简洁性

另一个工程师（或 agent）能否在作者不解释的情况下理解此代码？

• 命名是否描述性且与项目约定一致？（避免无上下文的 temp、data、result）
• 控制流是否直观（避免嵌套三元表达式、深层回调）？
• 代码是否逻辑有序（相关代码归组、清晰的模块边界）？
• 是否存在应该被简化的"巧妙"技巧？
• 能否以更少行数实现？（1000 行代码能用 100 行完成就是失败）
• 抽象是否对得起其复杂性？（不要泛化到第三个用例出现之前）
• 注释是否有助于澄清非显然的意图？（但不要注释显而易见的代码。）
• 是否存在死代码产物：无操作变量（_unused）、向后兼容垫片或 // removed 注释？
• 是否在不相关的流程中插入了一个新的条件分支？ 这是设计异味，而非琐碎问题——将逻辑推入自己的辅助函数、状态或策略中，而非纠缠到已有路径中。
• 是否出现对同一结构的重复条件判断？ 它们意味着缺失的模型或分发器。一个"临时"分支通常是永久性债务。

3. 架构

变更是否适合系统的设计？

• 是否遵循现有模式还是引入了新模式？如果引入新模式，是否有充分理由？
• 是否保持了清晰的模块边界？
• 是否存在应共享的代码重复？
• 依赖方向是否正确（无循环依赖）？
• 抽象层级是否适当（不过度工程，不过度耦合）？
• 此重构是减少了复杂性还是只是重新安置了它？ 统计读者理解此变更需要记住的概念数量。如果"更干净"的版本没有减少该数量，它就没有更干净——优先选择让整个分支、模式或层级消失的重构，而非将相同逻辑重新集中的重构。优先删除一个抽象而非打磨它。
• 是否将功能特定的逻辑泄漏到共享或通用模块中？ 将逻辑保持在它所属的层次，重用现有的规范辅助函数而非近乎重复的函数，不要将架构漂移正常化。
• 类型边界是否明确？ 质疑无谓的 any/unknown/optional/强制转换，以及掩盖不清晰不变量的静默回退——使边界明确通常会使周围的流程更简单。

4. 安全性

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

• 用户输入是否经过验证和清洗？
• 密钥是否远离代码、日志和版本控制？
• 是否在需要的地方检查了认证 / 授权？
• SQL 查询是否参数化（无字符串拼接）？
• 输出是否编码以防止 XSS？
• 依赖项是否来自可信来源并无已知漏洞？
• 来自外部来源（API、日志、用户内容、配置文件）的数据是否被视为不可信？
• 外部数据流在进入逻辑或渲染之前是否在系统边界进行了验证？

5. 性能

详细的性能分析与优化参见 performance-optimization。此变更是否引入了性能问题？

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

结构性修复

当你标记一个结构性问题时，提出移动方案——而不仅是问题本身。一个仅说"这很复杂"的评审让作者只能猜测。使用命名的重构方式：

• 用类型化模型或显式分发器替换条件链。
• 将重复分支收拢为单一的更清晰流程。
• 将编排与业务逻辑分离，使各自独立可读。
• 将功能特定逻辑移出共享模块，放入拥有该概念的那个包中。
• 重用规范的辅助函数，而非自定义的近乎重复的函数。
• 使类型边界显式化，从而使下游分支消失。
• 删除传递包装器，它为 API 添加了间接层次而未提供澄清。
• 提取辅助函数，或将大文件拆分为聚焦的模块。

优先选择减少活动部件的修复方案，而非将相同复杂性摊开的修复方案。

变更规模

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

约 100 行变更   → 良好。可一次阅读评审完毕。
约 300 行变更   → 若是单次逻辑变更可以接受。
约 1000 行变更  → 过大。请拆分。

关注文件大小，而不仅是 diff 大小。 小的 diff 仍然可能将文件推过合理边界——单个文件总共约 1000 行（区别于上文中约 1000 变更行的阈值）是一个常见的检查信号，而非硬性上限。当一次变更显著增长一个已经很大的文件时，应问自己：是否应先提取辅助函数、子组件或模块，再继续往上堆叠。先分解，再添加。

什么算"一次变更"： 一个自包含的修改，解决一件事，包含相关测试，且在提交后保持系统可用。这是功能的一部分——而非整个功能。

变更过大时的拆分策略：

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

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

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

变更描述

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

首行： 简短、祈使语气、独立。如"删除 FizzBuzz RPC"而非"正在删除 FizzBuzz RPC"。必须信息量足够，让搜索历史的人无需阅读 diff 即可理解此变更。

正文： 变更了什么以及为什么。包含代码本身不可见的上下文、决策和推理。链接到相关的 Bug 号、基准测试结果或设计文档。在存在不足时，承认方法上的欠缺。

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

评审流程

步骤 1：理解上下文

在审视代码之前，理解意图：

- 此变更试图达成什么？
- 它实现了什么规格或任务？
- 预期的行为变更是什么？

步骤 2：先评审测试

测试揭示了意图和覆盖范围：

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

步骤 3：评审实现

带着五维度的视角审视代码：

对每个变更的文件：
1. 正确性：此代码是否做到了测试所说的？
2. 可读性：我能否无需帮助就能理解？
3. 架构：此代码是否适合系统设计？
4. 安全性：是否存在漏洞？
5. 性能：是否存在瓶颈？

步骤 4：对发现进行分类

为每条评论标注严重级别，让作者清楚什么是必须的，什么是可选的：

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

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

优先处理最重要的内容。 按杠杆效应排序发现：正确性和安全性优先，然后是结构性退化和遗漏的简化，最后是其他。不要将真正的问题埋在表面性琐碎问题下——几个高确定性的评论胜过一长串细枝末节。如果你有一个结构性问题和十个琐碎问题，那个结构性问题就是评审的核心。

步骤 5：验证验证过程

检查作者的验证情况：

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

多模型评审模式

使用不同模型获取不同的评审视角：

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

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

评审 agent 的示例提示：

评审此代码变更的正确性、安全性，以及是否符合我们的项目约定。
规格描述是 [X]。此变更应达到 [Y]。
将问题标记为 Critical、Required、Optional 或 Nit。

死代码卫生

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

1. 识别现在无法访问或未使用的代码
2. 明确列出它们
3. 删除前先询问： "是否应该删除以下现在不再使用的元素：[列表]？"

不要让死代码闲置于此——它会迷惑未来的读者和 agent。但也不要悄悄删除你不确定的东西。如有疑问，先问。

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

评审速度

慢的评审会阻塞整个团队。上下文切换进行评审的成本低于强加给他人的等待成本。

• 在一个工作日内响应——这是最大限度，而非目标
• 理想节奏： 收到评审请求后尽快响应，除非处于深度编码中。一个典型变更应在一天内完成多轮评审
• 优先快速的个别响应而非快速的最终批准。快速反馈减少挫败感，即使需要多轮
• 大变更： 请作者拆分，而非评审一个庞大的一次性变更集

处理分歧

解决评审争议时，按以下层级处理：

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

不要接受"我之后会清理"。 经验表明延迟的清理很少发生。要求提交前清理，除非是真正的紧急情况。如果相关内容无法在本次变更中处理，要求提交一个 Bug 并自分配。

评审中的诚实

在评审代码时——无论是由你、另一个 agent 还是人类编写的：

• 不要盲从。 没有评审证据的 "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 代码需要更多审查，而非更少。它自信且合理，即使它是错误的。
"测试通过了，所以没问题"	测试是必要的但不充分。它们不能捕捉到架构问题、安全问题或可读性问题。
"这个重构让它更干净了"	重新安置复杂性不是减少复杂性。如果读者仍需记住相同数量的概念，结构就没有改进——寻找分支能够消失的版本。
"只是给这个文件加了一小点"	小的 diff 仍然能将文件推过健康大小，并向不相关的流程中添加分支。评判的是最终结构，而非 diff 大小。

红旗信号

• 未经任何评审的 PR 被合并
• 评审仅仅检查测试是否通过（忽略其他维度）
• 没有实际评审证据的 "LGTM"
• 安全敏感变更未经安全聚焦的评审
• "太大而无法正常评审"的大型 PR（请拆分）
• Bug 修复 PR 中无回归测试
• 评审评论无严重级别标注——让人分不清什么是必需的、什么是可选的
• 接受"我以后会修复"——它从未发生
• 移动代码但未减少读者需要掌握的概念数量的重构
• 生长已很大的文件而非分解它的变更
• 零散插入到不相关代码路径中的新条件分支（缺少抽象）
• 重复现有规范辅助函数的自定义助手函数，或放在共享模块中的功能逻辑

验证

评审完成后：

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

推定阻塞项： 对以下每项，暴露并提议更简单的设计；仅当变更实际恶化了结构时才升级为 Required：一个重新安置而非减少复杂性的重构；一个将文件推过大小边界而无分解的变更；添加到共享模块中的功能逻辑；对现有规范辅助函数的近乎重复的实现；隐藏不清晰不变量的静默回退。