vinvcn 代码审查和质量
概览
带质量门禁的多维度代码审查。每个变更在合并前都必须经过审查，没有例外。审查覆盖五个轴：正确性、可读性、架构、安全性和性能。
批准标准： 当一个变更明确改善了整体代码健康度时，就批准它，即使它并不完美。完美代码不存在，目标是持续改进。不要因为它和你自己的写法不完全一致就阻止它。如果它改善了代码库并遵循项目约定，就批准它。
何时使用
- 合并任何 PR 或变更之前
- 完成功能实现之后
- 当另一个 agent 或模型产出了你需要评估的代码时
- 重构现有代码时
- 修复任何 bug 之后（同时审查修复和回归测试）
五轴审查
每次审查都从这些维度评估代码：
1. 正确性
代码是否做了它声称要做的事？
- 是否符合 spec 或任务要求？
- 是否处理了边界情况（null、empty、边界值）？
- 是否处理了错误路径（不只是 happy path）？
- 是否通过所有测试？测试是否真的在测试正确的事情？
- 是否存在 off-by-one 错误、竞态条件或状态不一致？
2. 可读性和简单性
另一个工程师（或 agent）能否在作者不解释的情况下理解这段代码？
- 命名是否具有描述性，并与项目约定一致？（没有缺少上下文的
temp、data、result） - 控制流是否直接清晰（避免嵌套三元表达式、深层 callback）？
- 代码组织是否符合逻辑（相关代码放在一起，模块边界清晰）？
- 是否有应该简化的“聪明”技巧？
- 能否用更少的行数完成？（100 行足够却写了 1000 行就是失败）
- 抽象是否配得上它带来的复杂度？（不要在第三个用例之前泛化）
- 注释是否有助于澄清非显而易见的意图？（但不要注释显而易见的代码。）
- 是否存在死代码痕迹：no-op 变量（
_unused）、向后兼容 shim，或// removed注释？
3. 架构
这个变更是否适合系统设计？
- 它遵循现有模式，还是引入了新模式？如果是新模式，是否有充分理由？
- 是否保持了清晰的模块边界？
- 是否存在应该共享的代码重复？
- 依赖流向是否正确（没有循环依赖）？
- 抽象层级是否合适（不过度工程化，也不过度耦合）？
4. 安全性
详细安全指导见 security-and-hardening。这个变更是否引入了漏洞？
- 用户输入是否经过验证和清理？
- secrets 是否没有出现在代码、日志和版本控制中？
- 是否在需要的位置检查了认证/授权？
- SQL 查询是否参数化（没有字符串拼接）？
- 输出是否经过编码以防止 XSS？
- 依赖是否来自可信来源，且没有已知漏洞？
- 来自外部来源的数据（API、日志、用户内容、配置文件）是否被当作不可信数据处理？
- 外部数据流在进入逻辑或渲染之前，是否已在系统边界验证？
5. 性能
详细 profiling 和优化指导见 performance-optimization。这个变更是否引入了性能问题？
- 是否有 N+1 查询模式？
- 是否有无界循环或不受约束的数据获取？
- 是否有本应异步的同步操作？
- UI 组件是否存在不必要的 re-renders？
- 列表 endpoint 是否缺少 pagination？
- hot paths 中是否创建了大型对象？
变更大小
小而聚焦的变更更容易审查、更快合并，也更安全部署。目标大小如下：
~100 lines changed → Good. Reviewable in one sitting.
~300 lines changed → Acceptable if it's a single logical change.
~1000 lines changed → Too large. Split it.
什么算“一个变更”： 一个自包含的修改，只解决一件事，包含相关测试，并且提交后系统仍可运行。它是一个功能的一部分，而不是整个功能。
变更过大时的拆分策略：
| 策略 | 做法 | 何时使用 |
|---|---|---|
| Stack | 先提交一个小变更，再基于它开始下一个变更 | 顺序依赖 |
| By file group | 对需要不同审查者的文件组拆分变更 | 横切关注点 |
| Horizontal | 先创建共享代码/stubs，再接入消费者 | 分层架构 |
| Vertical | 将功能拆成更小的 full-stack 切片 | 功能开发 |
何时可以接受大变更： 完整删除文件，以及自动化重构。这类变更中，审查者只需要验证意图，而不是逐行检查。
将重构和功能开发分开。 一个既重构现有代码又添加新行为的变更，其实是两个变更，应分别提交。小型清理（例如变量重命名）可由审查者判断是否一起包含。
变更描述
每个变更都需要一段能在版本控制历史中独立成立的描述。
第一行： 简短、祈使句、可独立理解。写 "Delete the FizzBuzz RPC"，不要写 "Deleting the FizzBuzz RPC."。它必须足够有信息量，让搜索历史的人不读 diff 也能理解变更。
正文： 说明改变了什么以及为什么。包含代码本身看不出来的上下文、决策和推理。必要时链接 bug 编号、benchmark 结果或设计文档。如果方案存在不足，要明确承认。
反模式： "Fix bug," "Fix build," "Add patch," "Moving code from A to B," "Phase 1," "Add convenience functions."
审查流程
步骤 1: 理解上下文
看代码之前，先理解意图：
- What is this change trying to accomplish?
- What spec or task does it implement?
- What is the expected behavior change?
步骤 2: 先审查测试
测试会揭示意图和覆盖范围：
- Do tests exist for the change?
- Do they test behavior (not implementation details)?
- Are edge cases covered?
- Do tests have descriptive names?
- Would the tests catch a regression if the code changed?
步骤 3: 审查实现
带着五个轴逐步检查代码：
For each file changed:
1. Correctness: Does this code do what the test says it should?
2. Readability: Can I understand this without help?
3. Architecture: Does this fit the system?
4. Security: Any vulnerabilities?
5. Performance: Any bottlenecks?
步骤 4: 对发现的问题分类
为每条评论标注严重程度，让作者知道哪些是必需修改，哪些是可选建议：
| 前缀 | 含义 | 作者动作 |
|---|---|---|
| (no prefix) | 必需变更 | 合并前必须处理 |
| Critical: | 阻塞合并 | 安全漏洞、数据丢失、功能损坏 |
| Nit: | 轻微、可选 | 作者可以忽略，通常是格式或风格偏好 |
| Optional: / Consider: | 建议 | 值得考虑，但不是必须 |
| FYI | 仅供参考 | 无需动作，是供未来参考的上下文 |
这可以防止作者把所有反馈都当成强制要求，并在可选建议上浪费时间。
步骤 5: 验证其验证过程
检查作者的验证说明：
- What tests were run?
- Did the build pass?
- Was the change tested manually?
- Are there screenshots for UI changes?
- Is there a before/after comparison?
多模型审查模式
使用不同模型提供不同审查视角：
Model A writes the code
│
▼
Model B reviews for correctness and architecture
│
▼
Model A addresses the feedback
│
▼
Human makes the final call
这能捕捉单个模型可能漏掉的问题，因为不同模型有不同盲点。
审查 agent 的示例 prompt：
Review this code change for correctness, security, and adherence to
our project conventions. The spec says [X]. The change should [Y].
Flag any issues as Critical, Important, or Suggestion.
死代码卫生
任何重构或实现变更之后，都要检查孤立代码：
- 识别现在不可达或未使用的代码
- 明确列出它
- 删除前先询问： "Should I remove these now-unused elements: [list]?"
不要把死代码留在周围，它会迷惑未来的读者和 agent。但也不要默默删除你不确定的东西。有疑问就问。
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?
审查速度
缓慢的审查会阻塞整个团队。切换上下文进行审查的成本，低于让别人等待所造成的成本。
- 一个工作日内响应 — 这是上限，不是目标
- 理想节奏： 收到审查请求后尽快响应，除非正在深度专注编码。典型变更应在一天内完成多轮审查
- 优先快速给出单次反馈，而不是追求快速最终批准。即使需要多轮，快速反馈也能减少挫败感
- 大型变更： 要求作者拆分，而不是审查一个巨大的 changeset
处理分歧
解决审查争议时，按这个优先级处理：
- 技术事实和数据 高于观点和偏好
- Style guides 是风格问题的绝对权威
- 软件设计 必须基于工程原则评估，而不是个人偏好
- 代码库一致性 在不损害整体健康度时可以接受
不要接受“以后再清理”。 经验表明，推迟的清理很少发生。除非是真正紧急情况，否则要求在提交前清理。如果周边问题无法在本次变更中处理，要求创建 bug 并自我指派。
审查中的诚实
审查代码时，不管代码是你自己、另一个 agent 还是人类写的：
- 不要橡皮图章式批准。 没有审查证据的 "LGTM" 对任何人都没有帮助。
- 不要弱化真实问题。 一个会打到生产环境的 bug，不应被说成 "This might be a minor concern"。
- 尽可能量化问题。 "This N+1 query will add ~50ms per item in the list" 比 "this could be slow" 更好。
- 对明显有问题的方案提出反对。 迎合是审查中的失败模式。如果实现有问题，直接说明并提出替代方案。
- 优雅接受 override。 如果作者拥有完整上下文并不同意，尊重他们的判断。评论代码，不评论人；把个人化批评重构为聚焦代码本身。
依赖纪律
代码审查的一部分是依赖审查：
添加任何依赖之前：
- 现有技术栈能否解决这个问题？（通常可以。）
- 这个依赖有多大？（检查 bundle 影响。）
- 它是否仍在积极维护？（检查最近 commit 和 open issues。）
- 它是否有已知漏洞？（
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
### 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 代码需要更多审查，而不是更少。它即使错了，也会显得自信且合理。 |
| “测试通过了，所以没问题” | 测试是必要但不充分的。它们抓不到架构问题、安全问题或可读性问题。 |
危险信号
- PR 未经任何审查就合并
- 审查只检查测试是否通过（忽略其他轴）
- 没有实际审查证据的 "LGTM"
- 安全敏感变更没有安全专项审查
- 大到“无法正确审查”的 PR（拆分它们）
- Bug fix PR 没有回归测试
- 审查评论没有严重程度标签，导致必需项和可选项不清楚
- 接受“以后再修”，它永远不会发生
验证
审查完成后：
- 所有 Critical 问题已解决
- 所有 Important 问题已解决，或已明确说明理由并延期
- 测试通过
- Build 成功
- 验证说明已记录（变更了什么、如何验证）