code-review-and-quality

name	code-review-and-quality
description	进行多维度代码审查。在合并任何变更之前使用。在审查由你自己、另一个智能体或人类编写的代码时使用。当需要在代码进入主分支之前评估其跨多个维度的质量时使用。当需要评估变更的正确性、可读性、架构与性能，或审查 PR/diff 时使用。

代码审查与质量

概述

带有质量门禁的多维度代码审查。每个变更在合并之前都要经过审查——无例外。审查涵盖四个维度：正确性、可读性、架构和性能。

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

何时使用

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

五维度审查

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

1. 正确性

代码做了它声称要做的事情吗？

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

2. 可读性与简洁性

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

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

3. 架构

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

• 它遵循现有模式还是引入了新模式？如果是新的，是否有充分理由？
• 它是否维护了清晰的模块边界？
• 是否有应该共享的代码重复？
• 依赖方向是否正确（无循环依赖）？
• 抽象层次是否适当（不过度工程化，也不过紧耦合）？
• 这次重构是否减少了复杂度，还是仅仅移动了它？ 计算读者需要掌握的变更概念数量。如果一个"更干净"的版本没有改变这个数量，它就不更干净——优先选择能让整个分支、模式或层次消失的重组，而非仅仅是重新集中同样的逻辑。优先删除抽象而非打磨它。
• 功能特定的逻辑是否泄露到了共享或通用模块中？ 将逻辑保留在它所属的层次中，复用现有的规范辅助函数而非近乎重复的副本，不要将架构偏离正常化。
• 类型边界是否显式？ 质疑不必要的 any/unknown/可选/强制转换以及掩盖不清晰不变量的静默回退——使边界显式通常会让周围的流程控制变得更简单。

4. 性能

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

• 是否有 N+1 查询模式？
• 是否有无界循环或无约束的数据拉取？
• 是否有应该异步的同步操作？
• 热路径上是否有不必要的序列化/反序列化开销？
• 列表端点是否缺少分页？
• 热路径中是否创建了大对象？

结构性补救

当你标记一个结构性问题时，提出具体的改动方向——而不仅仅是问题本身。只说"这很复杂"的审查会让作者猜测。应提出有命名的重组方案：

• 用类型化模型或显式调度器替代条件链。
• 将重复分支合并为单一的更清晰流程。
• 将编排逻辑与业务逻辑分离，使各部分各自可读。
• 将功能特定逻辑移出共享模块，放入拥有该概念的包中。
• 复用规范辅助函数而非定制的近乎重复的副本。
• 使类型边界显式化，从而消解下游分支逻辑。
• 删除直通包装器，如果它增加间接层而没有澄清 API。
• 提取辅助函数，或将大文件拆分为专注的模块。

优先选择能消除活动部件的方案，而非将相同的复杂度摊开到更多的代码中。

变更规模

小而专注的变更更容易审查，合并更快，部署更安全。以这些规模为目标：

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

关注文件大小，而不仅仅是 diff 大小。 小的 diff 仍然可能将文件推过健康的边界——单个文件中约 1000 行的总行数（与上述约 1000 行变更行数的阈值不同）是一个常见的检查信号，而非硬性上限。当变更显著增长一个已经很大的文件时，应先考虑是否提取辅助函数、子组件或模块，再继续堆砌更多代码。先分解，再添加。

什么算"一个变更"： 一个自包含的修改，处理一件事，包含相关测试，并在提交后保持系统功能完整。它是功能的一部分——而非整个功能。

变更过大时的拆分策略：

策略	如何操作	何时使用
堆叠	提交一个小变更，在此基础上开始下一个	顺序依赖
按文件组	为需要不同审查者的文件组分拆变更	横切关注点
水平	先创建共享代码/桩，然后是消费者	分层架构
垂直	拆分为更小的完整功能切片	功能开发

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

将重构与功能工作分开。 一个既重构现有代码又添加新行为的变更是两个变更——分开提交。小型清理（变量重命名）可以根据审查者的判断包含在内。

变更描述

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

第一行： 简短、祈使语气、独立完整。"删除 FizzBuzz RPC"而不是"正在删除 FizzBuzz RPC"。必须足够有信息量，让搜索历史的人能在不阅读 diff 的情况下理解这个变更。

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

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

审查流程

步骤 1：理解上下文

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

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

步骤 2：先审查测试

测试揭示意图和覆盖率：

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

步骤 3：审查实现

带着五维度视角浏览代码：

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

步骤 4：分类发现

用严重程度标记每个评论，让作者知道什么是必须的、什么是可选的：

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

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

先讲重要的。 按影响力排序发现：正确性和安全性优先，然后是结构性退化和遗漏的简化，最后是其他所有内容。不要将真正的问题埋藏在表面的小问题之下——几个高确信度的评论胜过一长串列表。如果你有一个结构调整建议和十个小问题，结构调整本身就是审查的核心。

步骤 5：验证验证过程

检查作者的验证记录：

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

多模型审查模式

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

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

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

给审查智能体的示例提示词：

审查这段代码变更的正确性、安全性和对我们项目约定的遵守。
规格要求 [X]。变更应该 [Y]。
将所有问题标记为 Critical、Required、Optional 或 Nit。

死代码清理

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

1. 识别现在无法到达或未被使用的代码
2. 显式列出它们
3. 在删除之前询问： "我应该移除这些现在不再使用的元素吗：[列表]？"

不要让死代码到处散落——它会混淆未来的读者和智能体。但不要默默地删除你不确定的事情。有疑问时，先问。

已识别的死代码：
- internal/util/date.go 中的 FormatLegacyDate() ——已被 FormatDate() 替代
- src/storage/ 中的 OldBlockCache 类型 ——已被 BlockCache 替代
- src/config.go 中的 kLegacyApiUrl 常量 ——没有剩余的引用
→ 可以安全移除这些吗？

审查速度

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

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

处理分歧

解决审查争议时，应用以下层次：

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

不要接受"我之后会清理"。 经验表明，推迟的清理很少会发生。在提交之前要求清理，除非是真正的紧急情况。如果周围问题无法在此次变更中处理，要求提交一个缺陷并自分配。

审查中的诚实

审查代码时——无论是由你、另一个智能体还是人类编写的：

• **不要橡皮图章。**没有审查证据的"LGTM"对任何人都没有帮助。
• 不要淡化真正的问题。 将即将影响生产的缺陷描述为"这可能是个小问题"是不诚实的。
• 尽可能量化问题。 "这个 N+1 查询会给列表中的每个条目增加约 50ms"比"这可能很慢"更好。
• 对有明显问题的方法提出反对。 谄媚是审查中的失败模式。如果实现有问题，直接说出来并提出替代方案。
• 优雅地接受否决。 如果作者拥有完整上下文并且不同意，遵从他们的判断。评论代码，而非评论人——将个人批判重新聚焦到代码本身。

依赖项纪律

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

在添加任何依赖项之前：

1. 现有技术栈能解决这个问题吗？（通常可以。）
2. 依赖项有多大？（检查打包体积影响。）
3. 它是否在积极维护？（检查最近的提交、开放的 issue。）
4. 它是否有已知漏洞？（cargo audit / govulncheck ./... / 供应链扫描工具）
5. 许可证是什么？（必须与项目兼容。）

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

升级现有依赖项和其他代码变更一样，最危险的升级是那些批量合并的、带有"bump deps"之类消息的升级。用同样的纪律审查它们：

1. 阅读变更日志，而不仅仅是版本号。 Semver 是维护者可能没有兑现的承诺——一个"补丁"版本可能带有行为变更。对于主版本升级，阅读迁移说明并找出哪些地方会出错。
2. 每次一个依赖项。 逐个（或以小型相关组）升级和合并。当批量升级破坏构建时，你无法知道是哪个包导致的；单个包的变更能使原因显而易见，回滚干净利落。
3. 让测试来决定。 升级应该由之前和之后的绿色套件来验证，而非"它安装成功了"。如果该依赖项行为的测试覆盖较弱，这个空白本身就是真正的发现——先添加测试。
4. 关注传递依赖图。 大多数安装的包是没有人直接选择的。审查 lockfile 的 diff，而不仅仅是 Cargo.toml / go.mod；单次直接升级可能引入数十项间接变更。
5. 保持 lockfile 诚实。 提交它，审查它的 diff，永远不要手动编辑它。lockfile 才是真正锁定实际发布内容的文件。

对于筛选 cargo audit / govulncheck 发现和供应链风险（拼写仿冒、维护者被攻陷），请遵循项目的依赖审计策略——本节涵盖升级工作流；安全性裁决应由用户或项目安全策略决定。

审查检查清单

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

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

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

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

### 架构
- [ ] 遵循现有模式
- [ ] 无不必要的耦合或依赖项
- [ ] 抽象层次适当
- [ ] 重构减少了复杂度而非移动了它
- [ ] 共享模块中没有功能逻辑；文件保持在合理的大小范围内

### 安全性
- [ ] 代码中没有机密
- [ ] 输入在边界处进行了验证
- [ ] 无注入漏洞
- [ ] 认证检查到位
- [ ] 外部数据来源被视作不可信数据

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

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

### 裁决
- [ ] **批准**——可合并
- [ ] **要求变更**——问题必须处理

参见

• 性能审查检查点，见 references/performance-checklist.md

常见合理化借口

合理化借口	现实
"它能用，这就够了"	能用但不可读、不安全或架构错误的代码会产生复利债务。
"是我写的，所以我知道它是正确的"	作者对自己假设的盲点是不可避免的。每个变更都受益于另一双眼睛。
"我们之后会清理"	"之后"永远不会到来。审查就是质量门禁——使用它。要求在合并前清理而非合并后。
"AI 生成的代码应该没问题"	AI 代码需要更多而不是更少的审查。它自信且看似合理，即使错误时也是如此。
"测试通过了，所以没问题"	测试是必要的但不是充分的。它们不能捕获架构问题、安全问题或可读性问题。
"这次重构让它更干净了"	重新定位复杂度不是减少它。如果读者仍然需要掌握相同数量的概念，结构就没有改进——寻找能让分支消失的版本。
"这只是对文件的一个小补充"	小 diff 仍然可能将文件推过健康大小，并将分支拼接到不相关的流程上。评判结果结构，而非 diff 大小。
"这只是一个版本升级"	版本升级是你没有编写的行为变更。阅读变更日志；semver 不能保证没有破坏性。
"我在一个 PR 中升级所有东西以节省时间"	破坏构建的批量升级隐藏了哪个包导致的。每次一个依赖项，使原因和回滚都保持干净。

红旗警告

• PR 在未经任何审查的情况下合并
• 审查只检查测试是否通过（忽略其他维度）
• 没有实际审查证据的"LGTM"
• 安全敏感的变更没有安全聚焦的审查
• 太大的 PR "太大了无法正确审查"（拆分它们）
• 缺陷修复 PR 没有回归测试
• 审查评论没有严重程度标签——使人不清楚什么是必须的、什么是可选的
• 接受"我之后会修"——这永远不会发生
• 重构只是将代码四处移动而没有减少读者需要掌握的概念数量
• 增长一个已经很大的文件而非分解它的变更
• 新的条件分支散落在不相关的代码路径中（缺失的抽象）
• 重复现有规范辅助函数的定制辅助函数，或放在共享模块中的功能逻辑
• 没有变更日志审查且没有逐包隔离的批量"bump dependencies" PR
• 手动编辑、未提交或未经 diff 审查就合并的 lockfile 变更

验证

审查完成后：

• 所有 Critical 问题已解决
• 所有 Required（无前缀）变更已解决或明确推迟并有理由
• 测试通过
• 构建成功
• 验证记录已文档化（变更了什么，如何验证的）
• 依赖项升级已对照其变更日志进行了审查、按包隔离、并由绿色套件验证且 lockfile diff 已审查

推定阻塞项： 对以下每一项指出并提出更简单的设计；仅在变更主动使结构恶化时才升级为 Required：重构重新定位复杂性而非减少它；将文件推过大小边界而没有分解的变更；添加到共享模块的功能逻辑；现有规范辅助函数的近乎重复；隐藏不清晰不变量的静默回退。