# Code Review And Quality

> 执行多维度代码审查。在合并任何变更前使用。审查自己、其他智能体或人类编写的代码时使用。在代码进入主干分支前需要从多个维度评估代码质量时使用。触发词：代码审查、code review、代码评审、质量门、多维度审查、合并前审查、PR审查

- Skill: `kscz0000/code-review-and-quality` (Agent Skill)
- Install (CLI): `npx skillmds@latest add kscz0000/code-review-and-quality`
- Raw SKILL.md: https://api.skillmd.com/api/skills/kscz0000/code-review-and-quality/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- License: MIT
- Author: kscz0000 (https://skillmd.com/u/kscz0000)
- Updated: 2026-09-21
- Page: https://skillmd.com/skills/kscz0000/code-review-and-quality

---


# 代码审查与质量

## 概述

带质量门的多维度代码审查。每次变更在合并前都必须经过审查——没有例外。审查覆盖五个维度：正确性、可读性、架构、安全性和性能。

**通过标准：** 当一项变更确实提升了整体代码健康度时即予以通过，即便它并不完美。完美的代码并不存在——目标是持续改进。不要因为某项变更不完全符合你本人的写法而阻止它合并。如果它改善了代码库并遵循项目约定，就批准它。

## 何时使用

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

## 五维审查

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

### 1. 正确性

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

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

### 2. 可读性与简洁性

另一位工程师（或智能体）能否无需作者解释就理解这段代码？

- 命名是否具有描述性，并与项目约定保持一致？（不要出现脱离语境的 `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 行变更      → 良好。一坐就能审查完。
~300 行变更      → 可接受，前提是单条逻辑变更。
~1000 行变更     → 过大。应拆分。
```

**关注文件体积，而不仅仅是 diff 大小。** 一个小的 diff 仍可能把文件推过健康边界——单个文件约 1000 *总* 行（不同于上面 ~1000 *改动* 行的阈值）是一个常见的检查信号，而非硬性上限。当一项变更让一个已经很大的文件进一步显著膨胀时，先问问是否应先抽取辅助函数、子组件或模块，然后再往上堆。先分解，再添加。

**"一项变更"的判定标准：** 一项自包含的修改，处理一件事，附带相关测试，并在提交后保持系统可用。是特性的一部分，而不是整个特性。

**变更过大时的拆分策略：**

| 策略 | 方式 | 适用场景 |
|----------|-----|------|
| **堆叠** | 提交一项小变更，再基于它开始下一项 | 顺序依赖 |
| **按文件分组** | 针对需要不同审查者的文件组分别提交 | 横切关注点 |
| **水平** | 先创建共享代码/桩，再做消费者 | 分层架构 |
| **垂直** | 拆成更小的全栈切片来分步实现特性 | 特性开发 |

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

**把重构与特性工作分开。** 一项既重构现有代码又新增行为的变更等于两项变更——应分开提交。小型清理（变量重命名）可由审查者酌情纳入。

## 变更说明

每次变更都需要一份在版本控制历史中独立可读的说明。

**首行：** 简短、祈使句、独立成意。"Delete the FizzBuzz RPC" 而非 "Deleting the FizzBuzz RPC."。必须能让搜索历史的人无需阅读 diff 即可理解这次变更。

**正文：** 变更的内容与原因。包含代码本身无法看到的上下文、决策与推理。必要时链接缺陷编号、基准结果或设计文档。当方案存在不足时要承认。

**反模式：** "Fix bug"、"Fix build"、"Add patch"、"Moving code from A to B"、"Phase 1"、"Add convenience functions."

## 审查流程

### 第一步：理解上下文

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

```
- 这项变更要达成什么？
- 它实现了哪份规范或任务？
- 预期的行为变化是什么？
```

### 第二步：先审查测试

测试揭示了意图与覆盖度：

```
- 变更是否有对应的测试？
- 测试是否针对行为（而非实现细节）？
- 边界情况是否覆盖？
- 测试是否具有描述性命名？
- 如果代码变了，这些测试能否捕获回归？
```

### 第三步：审查实现

带着五维视角通读代码：

```
针对每个变更的文件：
1. 正确性：这段代码是否做了测试所要求的事？
2. 可读性：我能否不借助他人解释就理解它？
3. 架构：它是否契合系统？
4. 安全性：是否存在漏洞？
5. 性能：是否存在瓶颈？
```

### 第四步：对发现进行分类

为每条评论标注严重程度，让作者明白哪些必须改、哪些可选：

| 前缀 | 含义 | 作者动作 |
|--------|---------|---------------|
| *(无前缀)* | 必须修改 | 合并前必须处理 |
| **Critical:** | 阻塞合并 | 安全漏洞、数据丢失、功能损坏 |
| **Nit:** | 次要、可选 | 作者可忽略——格式、风格偏好 |
| **Optional:** / **Consider:** | 建议 | 值得考虑但非必须 |
| **FYI** | 仅供参考 | 无需动作——供未来参考的上下文 |

这能防止作者把所有反馈都当成强制项，而把时间浪费在可选建议上。

**把重要的事放在前面。** 按杠杆高低排序：正确性与安全优先，其次是结构性退化和错过的简化机会，最后是其他。不要把真正的问题埋在表面 nit 之下——几条高确信度的评论胜过冗长的清单。如果你只有一条结构性问题加十条 nit，那条结构性问题*就是*这份审查。

### 第五步：验证审查对象的验证手段

检查作者的验证故事：

```
- 运行了哪些测试？
- 构建是否通过？
- 是否做过手工测试？
- UI 变更是否有截图？
- 是否有改动前/后的对比？
```
## 多模型审查模式

让不同模型从不同视角进行审查：

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

这能捕捉单个模型可能漏掉的问题——不同模型有不同的盲区。

**给审查智能体的示例提示词：**
```
按正确性、安全性以及对我们项目约定的遵循度审查本次代码变更。
规范要求 [X]。本次变更应 [Y]。将任何问题标记为 Critical、Required、Optional 或 Nit。
```

## 死代码卫生

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

1. 识别现已不可达或不再使用的代码
2. 显式列出
3. **删除前先询问：** "应该移除以下这些现已无用的元素吗：[列表]？"

不要把死代码四处乱放——它会迷惑未来的读者和智能体。但也不要在拿不准时默默删除。拿不准就问。

```
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. **代码库一致性** 在不损害整体健康度的前提下是可接受的

**不接受"我稍后清理"。** 经验表明被推迟的清理很少真正发生。除非是真正的紧急情况，否则要求在提交前完成清理。如果周边问题在本次变更中无法解决，要求提交一个自行负责的缺陷单。

## 审查中的诚实

无论是审查自己、其他智能体还是人类编写的代码：

- **不要橡皮图章。** 没有审查证据的 "LGTM" 对任何人都没有帮助。
- **不要软化真正的问题。** 当它是一个会冲击生产环境的缺陷时，"这可能是个小问题"是不诚实的。
- **尽量量化问题。** "这次 N+1 查询会在列表中每项增加约 50ms" 优于 "这可能会慢"。
- **对有明显问题的方案提出反驳。** 谄媚是审查中的失败模式。如果实现有问题，请直接指出并提出替代方案。
- **优雅地接受推翻。** 如果作者掌握全部上下文并不同意，尊重其判断。评论针对代码而非人——将个人批评重新聚焦到代码本身。

## 依赖纪律

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

**在添加任何依赖之前：**
1. 现有技术栈是否已解决此问题？（通常是的。）
2. 该依赖有多大？（检查对 bundle 的影响。）
3. 它是否在积极维护？（查看最近提交、待处理 issue。）
4. 它是否存在已知漏洞？（`npm audit`）
5. 许可证是什么？（必须与项目兼容。）

**原则：** 优先选择标准库和既有工具，而非新依赖。每个依赖都是一笔负债。

## 审查清单

````markdown
## 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 代码需要更多而非更少的审视。它自信且貌似合理，哪怕错了也是如此。 |
| "测试都过了，所以没问题" | 测试必要但不充分。它们无法捕获架构问题、安全隐患或可读性顾虑。 |
| "这次重构让它更干净了" | 迁移复杂度不是降低它。如果读者仍然需要保留同样数量的概念，那结构并没有改善——去找让分支消失的那个版本。 |
| "只是给这个文件加了一点点" | 即便 diff 很小，仍可能把文件推过健康边界，或把分支硬挂到无关流程上。判断的是结果结构，而不是 diff 大小。 |

## 红旗

- PR 合并时没有任何审查
- 只检查测试是否通过的审查（忽略了其他维度）
- 没有实际审查证据的 "LGTM"
- 安全敏感变更未经过安全专项审查
- 因"太大而无法正确审查"而过大的 PR（应拆分）
- 缺陷修复 PR 缺少回归测试
- 审查评论没有严重程度标签——让人分不清哪些是必须、哪些可选
- 接受"我稍后修"——这种事不会发生
- 重构只是把代码搬来搬去，并没有减少读者需要保留的概念数
- 变更让一个已经很大的文件继续膨胀，而不是分解它
- 把新条件散落到不相关的代码路径中（缺失抽象的征兆）
- 写了一个近似重复的辅助函数而不是复用既有规范版本，或把特性逻辑放进共享模块

## 验证

审查完成后：

- [ ] 所有 Critical 问题已解决
- [ ] 所有 Required（无前缀）变更已解决或有正当理由显式推迟
- [ ] 测试通过
- [ ] 构建成功
- [ ] 验证故事已记录（变更内容、如何验证）

**预设拦截项：** 对每项给出更简洁的设计并浮出水面；仅当变更确实让结构变差时才升级为 Required：把复杂度搬走而不是降低的重构；让文件越过体积边界却不做分解的变更；往共享模块中加入特性逻辑；与既有规范辅助函数近似的复制品；掩盖模糊不变量的静默回退。

## 局限性

- 仅当任务清晰匹配其上游来源与本地项目上下文时才使用本技能。
- 在应用变更前，请验证命令、生成的代码、依赖、凭据以及外部服务的行为。
- 不要把示例当作环境专属测试、安全审查或用户对破坏性/高成本操作的批准之替代。