代码评审
概述
先找高风险问题，标出位置并给出可执行修复。
建议角色
- •🔍 代码评审者：识别逻辑缺陷、错误处理与可维护性。
- •🔒 安全专家：识别越权、注入、敏感信息泄露等。
何时使用
- •代码/PR/diff/补丁评审
- •关注 bug、安全、性能或可维护性风险
- •合并/审批前评审
不适用
- •未提供代码或 diff（先请求提供）
- •仅架构/设计讨论（用 architecture-review）
快速参考
| 关注点 | 信号 / 示例 |
|---|---|
| 正确性 | off-by-one、空指针、条件错误、缺少错误处理 |
| 安全 | 越权、SQL 注入、XSS/CSRF、泄露密钥 |
| 性能 | N+1 查询、无限循环、阻塞 I/O |
| 可维护性 | 命名不清、重复、分支复杂 |
| 最佳实践 | 框架约定、日志、错误处理 |
输出格式
- •先列问题，按严重度排序。
- •每个问题包含：位置（文件:行 或片段）、严重度、影响、修复。
- •然后写开放问题/假设。
- •最后 1-2 句总结；亮点可选。
- •若无问题，明确说明 + 残余风险/测试缺口。
- •缺上下文：只问一个关键问题；否则写明假设。
严重度标准
- •Critical：安全、越权、数据丢失、崩溃/恐慌
- •Important：逻辑 bug、性能回退、不稳定行为
- •Suggestion：可读性、命名、小型重构
最佳实践
评审方法
- •先扫后看：先快速扫描 diff 的全局，再深入细节
- •按严重度排序：优先报告 Critical 和 Important 问题
- •提供修复建议：不仅是指出问题，还要给出具体的修复代码或方向
- •解释影响：说明为什么这是一个问题，可能的后果
关注优先级
- •安全性：认证、授权、注入、数据泄露
- •正确性：逻辑错误、边界条件、错误处理
- •性能：N+1 查询、无限循环、资源泄漏
- •可维护性：命名、重复、复杂度、可读性
问题报告格式
每个问题应该包含：
- •位置：文件:行 或代码片段
- •严重度：Critical / Important / Suggestion
- •问题：清晰描述问题
- •影响：可能造成的后果
- •修复：具体可执行的修复建议
评审态度
- •建设性：目标是改进代码，而非批评作者
- •客观：基于证据和最佳实践，而非个人偏好
- •包容：理解作者可能有我不知道的上下文
- •谦逊：承认评审者也可能有盲区
特定场景
- •大型 PR：先识别主要变更，再深入细节
- •时间紧张：优先扫描安全、正确性、性能相关代码
- •不熟悉的领域：明确说明假设，请求领域专家评审
常见陷阱
- •只挑风格：忽略安全和正确性问题，只关注代码风格
- •不标位置：说"有问题"但不说在哪里
- •含糊建议：说"优化一下"但没有具体方向
- •态度傲慢：用命令式语言而非建设性建议
- •忽视上下文：不理解设计意图就批评
盒式评审
时间/权威压力下可限时，但仍需扫高风险区域（鉴权、输入、写入、并发），并说明已检/未检。
示例
markdown
问题 - b/api/users.ts:42 严重度: Critical 问题: 缺少鉴权/归属检查，任意用户可更新资料。 修复: 要求 `requireUser()` 并校验 `user.id` 与目标一致。 - b/db/users.ts:88 严重度: Important 问题: 无界查询在负载下会引发 N+1 行为。 修复: 增加 LIMIT 并批量获取关联数据。 问题/假设 - 该 handler 之前是否总有中间件设置 `req.user`？ 总结 - 鉴权检查加入前阻止合并；性能问题可后续处理。
常见错误
- •先写总结后写问题
- •缺少位置或严重度
- •忽略正确性/安全/性能只挑风格
- •修复建议含糊（“优化一下”）而无具体行动
借口 vs 事实
| 借口 | 事实 |
|---|---|
| “没时间标行号” | 没位置就没法修复；至少给文件与片段。 |
| “用户只要快速评审” | 快速也要先看高严重度问题。 |
| “要友好，别说问题” | 隐藏关键问题会造成真实伤害。 |
| “没发现就说看起来不错” | 明确无问题并列出残余风险或测试缺口。 |
红旗 - 立刻停止
- •“随便扫一眼就批准”
- •“只提风格意见”
- •“不标文件/行”
- •“只有总结，没有问题”
- •“未说明是否有问题”