代码评审规范

Code Review 标准、小 CL、评审礼仪

评审标准

规则

[Requirement] 所有代码变更必须经过至少 1 名同行评审（CR）并通过后才能合并到主干分支；紧急热修复需事后补评审。
[Requirement] 评审以”代码是否实现了正确的功能、是否可维护”为核心，不纠缠个人风格偏好；风格问题由 Lint/Formatter 自动校验。
[Requirement] 评审人必须在 2 个工作日内给出反馈；超过 2 个工作日未响应，作者可在群内提醒或请求重指派。
[Standard] 评审意见分为三类：必须修改（Blocking）、建议修改（Optional）、提问（Question）；只有 Blocking 类意见未解决时才阻止合并。
[Standard] 评审关注点优先级：正确性 > 安全性 > 可维护性 > 性能 > 风格 > 注释。
[Default] 评审人应默认信任作者的意图，针对问题给出建设性意见，而非直接否定。
[Suggestion] 对于涉及架构、安全、性能的重大变更，应指定 2 名以上评审人，其中至少 1 名为领域专家。
[Caution] 禁止”橡皮图章式”评审（LGTM 而未实际阅读代码），评审人须对其通过的变更负连带责任。

代码示例

# PR 描述模板（强制填写）
## 变更摘要
- 修复用户登录接口在并发场景下的竞态问题
- 引入分布式锁，依赖 Redis
 
## 关联需求
- JIRA-1234 登录偶发失败
 
## 自检项
- [x] 单元测试已更新，覆盖率达标
- [x] 无 Breaking Change
- [x] 已更新相关文档

# 评审意见标注格式
[Blocking] 第 45 行：这里直接拼 SQL 存在注入风险，请改用参数化查询。
[Optional] 建议抽取 `calculate_discount` 为独立函数，便于测试。
[Question] 这里为什么用 Redis 而不是本地缓存？是否有跨实例一致性要求？

反模式

# 反模式：无信息 LGTM
LGTM
 
# 反模式：人身攻击而非针对代码
你写的代码太差了，完全看不懂。
 
# 反模式：把评审当代码风格争吵
为什么用双引号不用单引号？（交给 Formatter，不要在评审中讨论）

小 CL 原则

规则

[Requirement] 单个 CL（Change List / PR）的变更行数建议控制在 400 行以内（不含生成代码、lock 文件、测试快照）；超过 800 行必须拆分。
[Requirement] 每个 CL 只做一件事：要么修 Bug，要么加功能，要么重构；混合多种目的的 CL 必须拆分。
[Standard] CL 标题应能准确概括变更内容，格式：<类型>: <简短描述>，例如 fix: 修复登录竞态、feat: 增加导出 Excel 功能。
[Standard] 大功能拆分策略：先提交基础设施（数据结构、接口定义），再提交核心逻辑，最后提交 UI/接入层；每个子 CL 独立可测试、可回滚。
[Default] 纯重构 CL 不应改变行为；若必须改变行为，应单独提交并在描述中说明。
[Suggestion] 若一个 Bug 修复涉及多个模块，可按模块拆分多个 PR，但须在描述中互相引用并标明合并顺序。
[Caution] 禁止”打包提交”——把不相关的小修小补塞进同一个 PR 以”凑数”，这会严重拖慢评审速度。

代码示例

# 正确：拆分为多个小 CL
PR #101: feat(user): 新增 UserRepository 接口定义
PR #102: feat(user): 实现 UserRepository 数据库层
PR #103: feat(user): 实现 UserService 业务逻辑
PR #104: feat(api): 接入 UserService 到 HTTP 路由
PR #105: docs: 更新用户模块 API 文档

# 正确：CL 描述清晰说明变更范围
标题：fix(order): 修复订单金额计算精度丢失
 
变更：
- 将 Decimal 计算从 float 迁移到 Decimal
- 新增 5 个边界测试用例
- 影响范围：订单服务 create_order / update_order 接口
 
风险评估：低，仅影响金额计算，已有完整测试覆盖

反模式

# 反模式：巨型 CL
标题：重构 + 新功能 + Bug 修复 + 依赖升级
变更文件：120 个，+5000 / -3000 行
 
# 反模式：一个 CL 做太多事
标题：更新用户模块
（实际包含：重构 UserService + 修复登录 Bug + 升级 fastapi 版本 + 改 README）

评审流程

规则

[Requirement] 流程：作者提交 PR → CI 自动检查通过 → 指派评审人 → 评审人反馈 → 作者修改 → 评审通过 → 合并。
[Requirement] CI 未通过的 PR 不得进入评审；作者须先修复 CI 问题（编译、Lint、测试、覆盖率）再请求评审。
[Requirement] 评审意见必须在 PR 中以评论形式给出，不得仅口头沟通；所有讨论和决策须留痕。
[Standard] 作者对每条评审意见须给出回应：① 已修改并附上 commit 链接 ② 说明不采纳的理由 ③ 标记为已解决。
[Standard] 若评审人与作者对某问题无法达成一致，升级到 Tech Lead 或架构组仲裁，不得在 PR 中反复拉锯。
[Default] 合并方式：Squash Merge 为主，保持主干提交历史简洁；涉及复杂分支拓扑时用 Merge Commit。
[Suggestion] 作者可在 PR 中标记 draft 状态表示 WIP，草稿 PR 不请求评审。
[Caution] 禁止作者在评审期间 force push 覆盖历史，除非评审人明确同意；修改应追加新 commit。

代码示例

# PR 状态流转
[Draft] → [Ready for Review] → [Changes Requested] → [Approved] → [Merged]
                                       ↓
                                  (作者修改后重新提交)

# GitHub PR 模板 .github/pull_request_template.md
## 变更类型
- [ ] Bug 修复
- [ ] 新功能
- [ ] 重构
- [ ] 文档
- [ ] 其他
 
## 变更描述
<!-- 简明描述本次变更的内容与目的 -->
 
## 关联 Issue
<!-- 例如：Closes #123 -->
 
## 自检清单
- [ ] CI 通过
- [ ] 单元测试覆盖新增逻辑
- [ ] 无敏感信息泄露
- [ ] 已更新文档（如需要）
 
## 评审人关注重点
<!-- 希望评审人特别关注的部分 -->

反模式

# 反模式：CI 未过就请求评审
作者："帮我看看这个 PR，CI 有个测试挂了但应该不影响"
（评审人被迫等待，浪费时间）
 
# 反模式：口头评审不留痕
评审人："这个地方改一下"（当面说了，PR 里没记录，事后无法追溯）
 
# 反模式：作者 force push 导致评论上下文丢失
评审人对 commit abc123 的评论在作者 force push 后变成悬空引用

评审礼仪

规则

[Standard] 评审意见对事不对人，使用中性、客观的语言描述问题，避免使用”你”做主语，改用”这里”、“这段代码”。
[Standard] 提出问题时尽量给出具体建议或替代方案，而非仅指出问题。
[Default] 对优秀的实现主动给予正面反馈（“这个抽象很清晰”），营造积极的评审氛围。
[Suggestion] 收到评审意见时先理解再回应；若不同意，用技术论据说明，不做情绪化反驳。
[Caution] 禁止在评审中使用攻击性、讽刺性语言；禁止因私人关系影响评审客观性。
[Caution] 评审人不应要求作者做超出本次 PR 范围的重构；若有更大范围的改进建议，记录为后续 Issue。

代码示例

# 推荐的评审意见写法
"这里循环中每次都查询数据库，建议先批量查询再在内存中过滤，参考 UserService.get_users 的实现。"
 
"这个函数的参数较多，是否考虑用一个 Config 对象封装，提高可读性？"
 
"异常处理覆盖了网络错误和超时，👍 但是没有处理 401 的情况，建议补充。"

# 推荐的回应写法
"已按建议改为批量查询，见 commit 5f3a2c1。"
 
"这里不改为 Config 对象是因为调用方已有 3 处，改动范围较大，已开 Issue #456 跟踪。"

反模式

# 反模式：人身攻击
"你怎么又写这种垃圾代码？"
"你到底懂不懂异步？"
 
# 反模式：只挑刺不给方案
"这里写得不好。"（不说明哪里不好、怎么改）
 
# 反模式：范围蔓延
"顺便把整个模块都重构一下吧。"（超出本次 PR 范围）
 
# 反模式：情绪化回应
"爱审不审，不审拉倒。"
"你说的都对，但我不改。"

检查清单

规则

[Requirement] 评审人在通过前必须逐项确认以下检查清单；作者在提交评审前也应自检。
[Standard] 检查清单分为 6 个维度：功能正确性、代码质量、安全性、性能、可测试性、文档与兼容性。
[Default] 对每一项，若不适用应在 PR 中明确说明”不适用”及原因。

代码示例

# 代码评审检查清单
 
## 功能正确性
- [ ] 变更实现了 PR 描述中声明的功能
- [ ] 边界条件（空值、极值、并发）已处理
- [ ] 错误处理路径完整，异常被正确捕获和传递
- [ ] 无逻辑漏洞（如 off-by-one、死循环、资源泄露）
 
## 代码质量
- [ ] 函数/类职责单一，无超长方法（>100 行需说明理由）
- [ ] 命名清晰，无语义模糊的变量名（tmp、data、obj 等）
- [ ] 无重复代码，公共逻辑已抽取
- [ ] 注释解释"为什么"而非"做什么"
 
## 安全性
- [ ] 无 SQL 注入、XSS、CSRF 风险（参数化查询、转义输出）
- [ ] 无敏感信息（密钥、密码、Token）硬编码或日志输出
- [ ] 输入已校验（类型、长度、格式、范围）
- [ ] 权限控制正确，未越权访问
 
## 性能
- [ ] 无 N+1 查询，数据库查询已优化索引
- [ ] 无循环中的 I/O 操作（网络/磁盘）
- [ ] 大数据量场景下内存使用合理（流式处理）
- [ ] 缓存策略合理，无缓存击穿/雪崩风险
 
## 可测试性
- [ ] 新增逻辑有对应的单元测试，覆盖率达标
- [ ] 依赖通过接口注入，便于 Mock
- [ ] 测试用例覆盖正常路径、异常路径、边界值
 
## 文档与兼容性
- [ ] API 变更已更新 OpenAPI/接口文档
- [ ] 数据库变更包含迁移脚本
- [ ] 无 Breaking Change，或已说明兼容方案
- [ ] 配置项变更已更新配置文档和示例

反模式

# 反模式：未走检查清单直接 LGTM
评审人看了 2 分钟，点 Approve，结果上线后发现有 SQL 注入。
 
# 反模式：只检查功能不检查安全
"功能没问题，过。"（忽略了日志中打印了用户明文密码）
 
# 反模式：检查清单流于形式
所有项都打勾，但实际没有认真核对。