review-code

name	review-code
description	Use when reviewing code changes, PRs, diffs, or commits. 触发：审查、review、CR、看看这段代码、检查这个 PR、审一下。
version	1.1.0
author	Hermes Agent
license	MIT
platforms	["linux","macos","windows"]
metadata	{"hermes":{"tags":["code-review","代码审查","review","quality","中文"],"related_skills":["requesting-code-review","github-code-review","code-review"]}}

代码审查 (Code Review)

Overview

基于 Google 工程实践整理的代码审查指南。首要目的：确保代码库的整体代码健康状况随时间推移而改善。 不存在"完美"代码，只有更好的代码——追求持续改善，而非追求完美。

评审者应在 CL 明确改善系统代码健康状态时予以通过，即使 CL 并不完美。

三大原则：

1. 技术事实和数据压倒意见和个人偏好。
2. 风格问题上，风格指南是绝对权威。 任何不在风格指南中的纯风格问题属于个人偏好，应保持与现有代码一致。
3. 软件设计几乎从来不是纯风格问题或个人偏好。 基于底层原则衡量，而非仅凭个人意见。

When to Use

使用场景：

• 审查代码变更、PR、diff、commit、patch
• 用户说"审查"、"review"、"CR"、"看看这段代码"、"检查这个 PR"、"审一下"
• 审计现有代码库或模块的代码质量
• 作为 requesting-code-review 审查阶段的参考标准

不适用：

• 纯文档变更、配置调整（除非涉及安全配置）
• 生成代码、数据文件、自动格式化的代码

与 requesting-code-review 的区别： 本技能定义审查标准和方法（审查什么、怎么看）。requesting-code-review 定义提交前的自动化流水线（安全扫描、基线对比、auto-fix 循环）。

审查总览

审查应覆盖以下维度：

维度	核心问题
设计	代码设计是否合理、与系统适配？
功能	行为是否符合作者意图？对用户是否有益？
复杂度	能否更简单？未来其他开发者能否轻松理解和修改？
测试	是否有正确、设计良好的自动化测试？
命名	变量、类、方法等命名是否清晰？
注释	注释是否清晰有用？注释应解释为什么而非是什么。
风格	是否遵循风格指南？
文档	是否同步更新了相关文档？
每一行	人工编写的每行代码都要看。数据文件、生成代码可略过。
上下文	在更广阔上下文中审视——整个文件、整个系统。不接受降低代码健康状态的 CL。

审查流程

三步法浏览 CL

第一步：整体视角

• 阅读 CL 描述，理解变更做什么、为什么做。
• 这个变更本身是否合理？如果根本不应该做，立即回复说明原因并建议替代方案。

第二步：审查核心部分

• 找到变更量最大的"主"文件，先审查这些核心部分。
• 如果发现重大设计问题，立即发送评论——不等其他部分审查完。如果设计问题严重，大量后续代码可能被废弃。

第三步：按合理顺序审查其余部分

• 确认无重大设计问题后，按逻辑顺序浏览剩余文件。
• 有时先读测试代码再读主代码有助于理解变更意图。

审查标准详解

设计审查（最重要）

• CL 中各代码模块的交互是否合理？
• 变更应该放在当前代码库还是抽到库中？
• 是否与系统其余部分良好集成？
• 现在是添加此功能的合适时机吗？

功能审查

• CL 是否实现了开发者意图？意图对用户是否有利？
• 思考边界情况、并发问题、站在用户角度思考。
• 特别关注：UI 变更（难以仅靠读代码判断影响）、并行编程（可能导致死锁或竞态条件）。

复杂度审查

逐级检查：单行 → 函数 → 类是否过于复杂？

"过于复杂" = 代码读者无法快速理解、调用或修改时容易引入 bug。

特别警惕过度工程：代码做得比需要的更通用，或添加了当前系统不需要的功能。解决现在已知的问题，而非猜测未来可能需要解决的问题。

测试审查

• 要求与变更匹配的单元测试、集成测试或端到端测试。测试应和产品代码在同一个 CL 中。
• 确保测试正确、合理、有用：
  • 代码损坏时测试会真的失败吗？
  • 底层代码变更时测试会误报吗？
  • 断言是否简单有用？
  • 不同测试方法之间是否合理分离？
• 测试也是需要维护的代码，不接受测试中的复杂度。

命名审查

好名字足够长以完全传达该项是什么/做什么，但不至于长到难以阅读。

注释审查

• 注释通常应解释为什么而非是什么。如果代码本身不够清晰，应该简化代码而非加注释。
• 例外：正则表达式和复杂算法通常需要注释解释它们在做什么。
• 查看 CL 之前的注释：是否有可移除的 TODO、是否有反对此变更的旧注释。
• 注释 ≠ 文档（类/模块/函数的文档应表达目的、用法和行为）。

风格审查

• 确保 CL 遵循相应风格指南。
• 想改进风格指南之外的内容，用 "Nit:" 前缀标注，表示非强制性。
• 不要仅基于个人风格偏好阻止提交。
• CL 不应将大规模风格变更与功能变更混在一起——格式化和功能应分开提交。

一致性

• 如果现有代码与风格指南不一致：风格指南是绝对权威，CL 应遵循指南。
• 如果风格指南是建议而非要求：倾向于遵循风格指南，除非局部不一致会造成更大混淆。

文档审查

• 如果 CL 改变了用户构建、测试、交互或发布代码的方式，检查是否同步更新了 README、文档页面和生成的参考文档。
• 如果 CL 删除或废弃代码，考虑文档是否也应删除。

评论写法

三项要点

1. 解释原因——让开发者理解评论的意图、遵循的最佳实践、或建议如何改善代码健康。
2. 平衡直接指导和指出问题让开发者自己决定——指出问题让开发者决策，但有时直接指令、建议甚至代码更有帮助。
3. 鼓励开发者简化代码或添加代码注释，而非依赖于在审查工具中做解释。审查工具中的解释对未来代码读者没有帮助。

评论严重程度标签

标签	含义
Nit	小事，技术上应该做但影响不大
Optional / Consider	可能是个好主意，但不严格要求
FYI	不期望在本 CL 中修改，但未来可考虑

使用标签让审查意图明确，帮助作者确定优先级。

关于解释

如果要求开发者解释一段代码，通常结果应该是他们重写代码使其更清晰。偶尔添加代码注释也是合适的回应（前提是不是仅仅解释过于复杂的代码）。审查工具中写的解释对未来代码读者没有帮助。

处理"以后再清理"

开发者常表示会在后续 CL 中清理——经验表明，除非在立即写清理 CL，否则很少真正发生。

• 通常最好坚持开发者在代码进入代码库之前现在清理。"以后再清理"是代码库退化的常见方式。
• 如果 CL 引入了新的复杂度，必须在提交前清理（除非紧急情况）。
• 如果 CL 暴露了周围问题且现在无法解决，开发者应提交 bug 并分配给自己，可选在代码中写 TODO 引用该 bug。

CL 指南

写好 CL 描述

CL 描述应传达：做了什么变更？ + 为什么做这些变更？

第一行规则： 简短摘要（祈使句），独立存在让未来搜索代码历史的人能快速理解。第一行后空一行。

坏的描述： "Fix bug." / "Fix build." / "Add patch." / "Phase 1."

好的描述：

RPC: 移除 RPC 服务器消息空闲列表的大小限制。

像 FizzBuzz 这样的服务器有非常大的消息，能从复用中受益。
增大空闲列表，并添加一个 goroutine 随时间缓慢释放空闲列表条目，
使空闲服务器最终释放所有空闲列表条目。

写小 CL

一个 CL = 一个自包含的变更，只解决一件事。通常 100 行合理，1000 行太大。文件分散程度也影响"大小"——一个文件中的 200 行可能 OK，散在 50 个文件中通常太大。

拆分策略：

• 基于堆叠：写完一个小 CL 发出审查后，立即基于此 CL 写下一个。
• 按文件拆分：需要不同评审者的文件分组，各自自包含。
• 水平拆分：在技术栈各层之间创建共享代码或桩，隔离变更。
• 垂直拆分：按功能拆分，每个功能是独立的全栈实现。
• 重构分离：重构独立 CL，不与功能变更或 bug 修复混在一起。
• 测试在同一个 CL：逻辑变更应包含新的或更新的测试。

AI 审查执行指南

审查准备

git diff --cached
# 如果为空
git diff HEAD~1 HEAD

审查输出格式

## 审查总结
[1-2 句整体评估]

## 设计层面
- [设计相关评论]

## 关键问题（必须修复）
- 问题 1：[描述 + 建议修复方案]
- 问题 2：...

## 建议（可选）
- 建议 1：[描述]

## 好的实践（肯定）
- [开发者做得好的地方]

## 问题
- [需要开发者澄清的问题]

常见问题速查

Python

# 差: SQL 注入风险
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")
# 好: 参数化查询
cursor.execute("SELECT * FROM users WHERE id = ?", (user_id,))

# 差: Shell 注入
os.system(f"ls {user_input}")
# 好: 安全子进程
subprocess.run(["ls", user_input], check=True)

# 差: 危险 eval/exec
eval(user_input)
# 好: 使用安全的解析方式

JavaScript

// 差: XSS 风险
element.innerHTML = userInput;
// 好: 安全文本内容
element.textContent = userInput;

需要特别关注的信号

• 硬编码的密钥、API Key、凭证
• 缺少输入验证
• SQL 查询使用字符串拼接
• 文件操作未验证路径（路径穿越风险）
• 外部调用缺少错误处理
• 遗留的调试 print/console.log
• 被注释掉的代码
• 新代码没有对应测试
• 过度工程（做了当前不需要的抽象/功能）
• 并发代码没有正确处理竞态条件
• 重构和功能变更混在同一个 CL 中
• 大规模格式化变更和逻辑变更混在一起

Common Pitfalls

1. 只关注表面差异——应审查变更的意图和设计，而非只是 diff 的语法正确性。
2. 跳过测试审查——测试的质量和覆盖率同等重要，测试也是代码。
3. 接受"以后再清理"——几乎不会发生。坚持现在处理。
4. 风格偏好冒充设计问题——区分风格（遵循风格指南）和设计（基于原则权衡）。不确定时用 Nit 标签。
5. 忽略过度工程——"以后可能需要"不是添加抽象的理由。解决现在的问题。
6. 大 CL 一把审查——超过 1000 行应要求拆分，否则审查质量无法保证。
7. 功能和重构混在一起——格式/重构变更应独立于功能变更，分开提交。

Verification Checklist

• CL 描述清晰传达了做什么和为什么
• 审查了全部 10 个维度（设计、功能、复杂度、测试、命名、注释、风格、文档、每一行、上下文）
• 没有降低代码健康状态的变更被接受
• 复杂度和过度工程已评估
• 测试覆盖充分、测试设计合理
• 安全信号已检查（密钥、注入、路径穿越等）
• 评论带有严重程度标签（Nit/Optional/FYI）
• "以后再清理"已被拒绝（除非有立即的后续 CL）
• 输出格式遵循标准模板（审查总结 → 关键问题 → 建议 → 好的实践 → 问题）