kscz0000 代码审查与质量
概述
带质量门的多维度代码审查。每次变更在合并前都必须经过审查——没有例外。审查覆盖五个维度：正确性、可读性、架构、安全性和性能。
通过标准： 当一项变更确实提升了整体代码健康度时即予以通过，即便它并不完美。完美的代码并不存在——目标是持续改进。不要因为某项变更不完全符合你本人的写法而阻止它合并。如果它改善了代码库并遵循项目约定，就批准它。
何时使用
- 合并任何 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。
死代码卫生
任何重构或实现变更之后，检查孤立代码：
- 识别现已不可达或不再使用的代码
- 显式列出
- 删除前先询问： "应该移除以下这些现已无用的元素吗：[列表]？"
不要把死代码四处乱放——它会迷惑未来的读者和智能体。但也不要在拿不准时默默删除。拿不准就问。
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?
审查速度
慢审查会阻塞整个团队。上下文切换去做审查的成本低于给别人带来的等待成本。
- 一个工作日内响应 ——这是上限，而不是目标
- 理想节奏： 审查请求一到就尽快响应，除非你正在深度专注编码。典型变更应在一天内完成多轮审查
- 优先快速给出单次反馈 而非快速最终批准。快速反馈即使多轮也能降低挫败感
- 大变更： 让作者拆分，而不是去审查一份庞大的变更集
处理分歧
解决审查争议时，遵循以下层级：
- 技术事实与数据 凌驾于意见与偏好之上
- 风格指南 是风格问题的绝对权威
- 软件设计 必须以工程原则而非个人偏好来评估
- 代码库一致性 在不损害整体健康度的前提下是可接受的
不接受"我稍后清理"。 经验表明被推迟的清理很少真正发生。除非是真正的紧急情况，否则要求在提交前完成清理。如果周边问题在本次变更中无法解决，要求提交一个自行负责的缺陷单。
审查中的诚实
无论是审查自己、其他智能体还是人类编写的代码：
- 不要橡皮图章。 没有审查证据的 "LGTM" 对任何人都没有帮助。
- 不要软化真正的问题。 当它是一个会冲击生产环境的缺陷时，"这可能是个小问题"是不诚实的。
- 尽量量化问题。 "这次 N+1 查询会在列表中每项增加约 50ms" 优于 "这可能会慢"。
- 对有明显问题的方案提出反驳。 谄媚是审查中的失败模式。如果实现有问题，请直接指出并提出替代方案。
- 优雅地接受推翻。 如果作者掌握全部上下文并不同意，尊重其判断。评论针对代码而非人——将个人批评重新聚焦到代码本身。
依赖纪律
代码审查的一部分是依赖审查：
在添加任何依赖之前：
- 现有技术栈是否已解决此问题？（通常是的。）
- 该依赖有多大？（检查对 bundle 的影响。）
- 它是否在积极维护？（查看最近提交、待处理 issue。）
- 它是否存在已知漏洞？（
npm audit） - 许可证是什么？（必须与项目兼容。）
原则： 优先选择标准库和既有工具，而非新依赖。每个依赖都是一笔负债。
审查清单
## 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：把复杂度搬走而不是降低的重构；让文件越过体积边界却不做分解的变更；往共享模块中加入特性逻辑；与既有规范辅助函数近似的复制品；掩盖模糊不变量的静默回退。
局限性
- 仅当任务清晰匹配其上游来源与本地项目上下文时才使用本技能。
- 在应用变更前，请验证命令、生成的代码、依赖、凭据以及外部服务的行为。
- 不要把示例当作环境专属测试、安全审查或用户对破坏性/高成本操作的批准之替代。