pr-review

PR 评审

此技能涵盖聚焦于特定视角的拉取请求评审：选择合适的切入角度（安全性、性能、测试或架构），给出具体且按严重程度排序的问题，并以明确的合并结论收尾。

评审 PR 的工作流

1. 选择角度 — 根据用户的请求确定重点（“security”“perf”“tests”“arch”）。如果未指定，请询问要采用哪个角度，或默认采用风险最高的安全性角度。
2. 获取完整文件上下文 — 仅查看差异通常会错过变更行附近的缺陷。如果只有差异内容，请在做出确定判断前索取相关文件的上下文。
3. 根据角度的检查清单进行评审 — 按优先级逐项检查下方相关清单；除非用户要求全面评审，否则不要混合不同角度。
4. 写出具体且附有引用的问题 — 每个问题都要指出文件和行号，并说明具体缺陷，而不是模糊的印象。
5. 按严重程度排序 — 将问题分为 blocker、important 或 nit。
6. 明确说明不确定性 — 如果差异内容不足以确定，请说明这一点，并索取更多代码，不要猜测。
7. 单独一行给出结论 — Safe to merge | needs changes | reject（或下方针对架构的结论）。

输出规范

适用于所有评审角度：

• 为每个问题标注文件路径和行号。
• 按严重程度排序问题：blocker、important、nit。
• 具体说明。“这看起来有风险”不算问题；“src/auth.ts:42 — JWT 密钥从请求正文中读取，见第 41 行”才是问题。
• 如果差异内容不足以确定，请明确说明并索取相关文件的上下文。
• 单独一行给出结论：Safe to merge | needs changes | reject。

角度 1：安全性

按优先级检查 PR 中的安全缺陷：

1. 身份验证/授权 — 新增端点或分支缺少身份验证检查、对角色的假设、IDOR（不安全的直接对象引用）。
2. 输入验证 — 不可信输入流入查询、Shell 命令、文件路径、反序列化或 eval。
3. 注入 — SQL、NoSQL、命令、提示词注入、模板注入。
4. 密钥 — 硬编码的密钥/令牌、日志中的密钥、打包进客户端代码的密钥、提交到仓库的 .env。
5. 输出编码 — 未转义模板导致的 XSS、用户内容中的原始 HTML、JSONP 风格的信息泄露。
6. 加密/随机性 — 使用 Math.random() 生成令牌、将 MD5/SHA1 用于密码或完整性校验、缺少 IV、自行编写加密方案。
7. 数据暴露 — 日志中包含 PII、API 响应分享过多信息、缺少数据脱敏。

跳过锦上添花的建议；只关注缺陷。

角度 2：性能

检查是否存在性能回归：

1. N+1 模式 — 循环中针对每个条目分别进行数据库或网络调用，而不是批量处理。
2. 热路径分配 — 在循环中创建新的对象/数组/映射，或每次调用都重新编译正则表达式。
3. 无界工作量 — 缺少分页、结果集不受限制、递归没有深度上限。
4. 异步处理不当 — 应使用 Promise.all 却顺序执行多个 await，或缺少并发限制。
5. 缓存误用 — 缓存键遗漏相关变量、TTL 缺失或设置不合理。
6. 算法复杂度 — 隐含的 O(n²)（例如在 .map() 中调用 .some()）、在循环中排序。

引用具体行号，指出复杂度类别或不良模式，并提出修复建议。

角度 3：测试

审查 PR 的测试覆盖情况：

1. 新代码路径的测试——每个新增分支都应至少有一个测试。
2. 边界情况——空输入、null/undefined、边界值、依赖项抛出的错误。
3. 断言强度——错误值也能通过的断言、仅使用快照的测试、只检查成功路径的测试。
4. 模拟规范——真实接口变更时不会失败的模拟，以及会掩盖真实行为的过度模拟。
5. 确定性——日期/时间/随机数/网络未设为桩，导致测试不稳定。
6. 测试名称——名称未描述被测行为。

测试存在，不等于测试能捕获回归问题——要看断言，而不只是测试名称。

角度 4：架构

从逐行细节中抽离出来，审查变更的整体形态：

1. 边界漂移——层与层之间的界线发生了什么变化？UI 是否开始直接访问数据库？领域类型是否开始导入传输层类型？
2. 过早抽象——只有一种实现的接口、工厂或配置层；请将其标记为技术债。
3. 耦合——工具函数现在是否导入了功能模块？是否引入了共享可变状态？
4. 可扩展性——如果这条代码路径的规模扩大 10 倍，最先会出什么问题？
5. 可逆性——如果一个月后发现这是错的，回滚有多难？明确指出不可逆的决策。
6. 命名——类型/函数是否按其实现命名（UserManagerImplV2），而不是按其职责命名（UserDirectory）。

最后以此结尾：架构合理 | 需要精简 | 合并前需重新思考。

示例问题格式

src/api/orders.ts:118 (blocker, security)
  Order lookup uses `req.params.orderId` directly in a WHERE clause
  built via string concatenation — SQL injection. Use a parameterized
  query or the existing `db.orders.findById()` helper instead.

src/api/orders.ts:142 (nit, tests)
  `createOrder` has no test for the case where `items` is an empty
  array; current tests only cover the happy path with 1-3 items.

Verdict: needs changes

同时审查多个角度

• 当用户要求“全面审查”而非某个特定角度时，依次运行全部四项检查清单，而不是把它们混在一起进行一轮笼统审查——每个角度都会捕捉不同类型的缺陷，分开呈现也便于追溯问题所属的关注点。
• 将每个角度分别放在自己的标题下，并给出各自的结论行；不要把四个结论合并成一句总结。
• 如果时间或上下文有限，优先级应为安全性、测试、性能、架构——安全缺陷一旦发布，其严重性远高于架构上的瑕疵。

审查中的常见陷阱

• 根据差异的意图（“看起来是增加了验证”）就批准，而不是阅读代码的实际行为。
• 把测试套件通过当作正确性的证明——测试套件只能证明现有测试通过，不能证明所需的测试都已编写。
• 让审查范围不断扩大：可以单独提及差异未涉及的既有问题，但不应因此阻止正在审查的 PR。
• 未充分了解相关文件，就给出结论——应要求提供更多上下文，而不是猜测。