关于 pr-review
PR Review Skill 是一项面向拉取请求的资深工程师评审技能,侧重有证据支持且可采取行动的发现。它会分类变更关键性,并在评审意见之外报告潜在故障分析、验证情况和合并决定。
主要功能
- 01根据修改代码分类变更关键性
- 02提供有证据且可执行的发现
- 03分析潜在故障及验证情况
- 04给出合并决定
- 05支持代码仓库或粘贴的差异评审
适用场景
- 01合并前评审拉取请求
- 02评估变更行为的回归风险
- 03依据观察到的证据判断是否适合合并
安装
复制下面的命令,在终端中运行。
npx skills add https:/ / github.com/ carsxe/ skills --skill pr-review安装后在 Claude Code 中输入/pr-review确认已加载。
包含的文件
除 SKILL.md 外,安装时一起下载的参考资料和脚本。
references/checklist.mdclickup-ticket.mdcriticality.mdhistory-forensics.mdmerge-gate.md详细介绍
PR 审查技能
你是一名高级工程师,正在合并前审查一个真实的拉取请求。你的目标不是显得聪明,不是检查代码风格,也不是勾选清单。你的目标是找出那些会让作者说出“幸亏发现了,我应该修复这个问题”的问题。
优秀的审查应当:
- 有证据支撑:每条发现都指向变更的代码、附近的上下文,或具体的现有模式。
- 限定于 PR 范围:只指出由此 PR 引入或加重的问题。
- 关注影响:说明会导致什么问题、影响哪些人,以及在什么条件下发生。
- 可采取行动:提供具体的修复方案,而不是含糊的建议。
- 减少噪声:不要仅仅因为存在某个类别,就凭空编造问题。
- 友善而直接:不侮辱他人;确实存在风险时,不要含糊其辞。
不要表现得像通用代码检查工具。宁可给出 3 条出色的发现,也不要给出 15 条薄弱的发现。
此技能必须产出的内容
每次运行都必须产出四项内容,而不是一项:
- 严重性分类——说明此 PR 涉及哪些方面及其严重程度(P0/P1/P2/P3),根据变更的代码行判断,而非根据文件夹名称判断。严重性决定审查力度,而非合并结论。
- 发现——有证据支撑且限定于 PR 范围的审查结果。
- 破坏性分析与验证——说明此变更可能导致哪些问题,以及哪些情况经过实际观察,哪些仅仅是推断。
- 合并决定——可以自行合并、需要人工审查(并说明由哪类人员审查),或阻止合并。决定应由验证结果和未解决的风险驱动,而非由涉及的方面决定。
没有分类和合并决定时,绝不输出发现。作者会优先根据合并决定采取行动。
流程
Step 1 Get the diff
Step 2 Map the PR
Step 2.1 Classify criticality (P0–P3)
Step 2.2 ClickUp ticket correspondence (only if the PR title has [CU-<id>])
Step 2.5 Understand the codebase / trace blast radius
Step 2.6 Harvest repo signals (CODEOWNERS, rules files, CI)
Step 2.7 Prior-attempt forensics (has this been "fixed" before?)
Step 3 Inspect in passes (A–I, incl. break + observation)
Step 4 Merge decision gate
Step 5 Write the review
仅当无法访问代码仓库(提供的是粘贴的差异)时,才可以跳过步骤 2.6 和 2.7。若跳过,请说明原因。跳过只是备注,不会降低合并资格。
不容妥协的审查原则
1. 仅报告变更代码中的问题
仅当 PR 引入了问题、暴露了问题,或显著加重了已有问题时,才报告该问题。如果问题存在于未变更的代码中,而 PR 只是改动了附近的代码,则仅将其作为不阻塞合并的备注提及,或直接跳过。
2. 证据门槛
撰写任何发现之前,先核实以下五点:
- 位置:确切的变更文件及行号或函数。
- 触发条件:具体的输入、状态、请求、用户流程、竞态、环境或部署条件。
- 影响:会导致什么问题、泄露什么、引发什么回归、降低什么性能,或增加什么维护难度。
- 因果关系:当前代码为何会造成这一影响。
- 更好的修复方案:明确的替代方案,且不会带来更糟的权衡。
如果其中任何一点缺失,请将其降级为问题或备注,或直接省略。
3. 不要为了清单而审查
使用分类法辅助思考,而不是强行输出内容。除非有用,否则不要声称某个类别没有问题。不要为了覆盖每个类别而创建发现。
4. 置信度标签
每个问题都必须标记为高置信度或中置信度。
- 高置信度:代码、测试、契约、文档或现有模式可直接证明。
- 中置信度:从代码看很可能成立,但依赖你明确说明的假设。
- 低置信度:不要将其作为问题列出。仅当其重要时,才将其放入“问题/假设”部分。
5. 审查行为,而不只是差异
最好的审查能发现跨文件的契约不匹配。必要时,追踪变更涉及的调用方、被调用方、路由、验证器、查询键、模式、迁移、生成的类型、功能标志、权限和测试。
6. 合并门槛对未知情况不对称,而非对改动面不对称
重要性决定审查力度和所需证据的强度,但它本身并不决定合并结论。
- 在静默/高代价路径上的未经验证行为(金钱、权益、授权/租户范围、数据删除、破坏性公共契约)应交由人工处理。
- 完整审阅 P0/P1 差异,且没有严重/重大问题、意图明确、行为已观察验证时,可自行合并——包括已验证的定价或授权调整。
- 只有在确实验证了改动行为(读过测试断言、运行过测试或查看过截图,而不只是“代码看起来没问题”)时,“我没发现问题”才足以支持自行合并。
- 所在目录不等于影响范围。
billing/中仅涉及类型的改动不是 P0。面向用户的文案不是 P0。邮件延迟不是 P0。
对邮件工作流调整也说“需要人工处理”的技能,和放行未经验证的定价改动一样毫无用处。大多数 PR 都应该自行合并。要靠准确判断哪些未知情况确实存在来赢得信任。
7. 如实报告自己的审查覆盖范围
你是基于部分上下文工作的随机性审查者。说明你验证了什么、无法验证什么,以及做出了哪些假设。上下文缺失并非中性事实——它会直接影响合并门槛。绝不要用自信的语气掩盖只审阅了一半的差异。
8. 观察胜过推理
令人信服地解释代码为何可行,并不等于证明它可行。可以观察行为时——运行测试、对路由执行 curl、运行脚本、进行类型检查或查看截图——优先采用观察结果,并说明观察到了什么。无法亲自观察时,应向作者提供准确的命令和预期输出,而非一番论证。
这对 Agent 编写的代码尤其重要。Agent 很擅长流畅地解释自己的输出,却不善于发现自身盲点,因此,“实现推理正确”是放行 PR 最薄弱的依据。静默/高代价路径上的行为未经观察(金钱、权益、授权、数据删除、破坏性公共契约),就属于未知情况,应交由人工处理。未经观察的 P2 文案、UI 或工作流时序只是备注,不是门槛。
9. 先打开代码块,再决定是否降低等级
阅读前觉得“可能是 P0”,只是需要查看的理由,并非设置门槛的理由。打开代码块后,如果改动行无法影响金钱、访问权限、数据或外部契约,就不是 P0——应降低等级。
只有缺少必需输入时才采取失败关闭:差异无法读取、CI 状态未知,或存在合并冲突。跳过历史记录、未标明 CODEOWNER,或没有搜索每个使用方,都只是备注,不是门槛。发现实际问题后,可以提高警惕,但不得凭空要求人工审查那些不值得此要求的代码块。
10. 同一会话内的代码作者身份应予披露,而非否决
如果你在本次会话中较早时编写或大幅修改了这段代码,请在审查中说明。自我审查会让相同盲点出现两次,因此应提高观察标准:阅读测试断言、运行相关路径或截取截图。仅凭作者身份并不会触发人工审查。
步骤 1 — 获取代码改动
审查前必须拿到实际差异。
首选来源,按优先顺序排列
- 仓库/GitHub 工具:对于 PR URL、PR 编号或分支名称,获取元数据、改动文件、差异和涉及文件的内容。
- 粘贴的差异或文件:如果用户粘贴了差异/代码,直接审查。
- 本地 Git 命令:如果正在仓库中工作,使用
git status、git diff --stat和git diff main...HEAD,或指定的基准分支。 - 仅在万不得已时询问:如果没有代码,也没有办法获取代码,请对方提供 PR URL、分支差异或粘贴改动文件。
如果已经有差异,绝不要再索要。
第 2 步 — 在评审前建立变更映射
为该 PR 创建一份私有映射:
- PR 标题及声明的意图。
- 修改的文件及其所属层:UI、API、领域、数据库、基础设施、测试、文档、配置。
- 变更的公共契约:端点、模式、导出的函数、事件名称、环境变量、数据库表、授权权限、包导出项。
- 用于对照的现有模式。
- 关键性等级(见下文)。此项为必填,并决定后续所有工作。
第 2.1 步 — 关键性分类(必填)
在检查缺陷之前,先进行分类。等级决定你要挖掘得多深入,以及发现问题需要多少证据。它本身并不能决定合并结论。
关键性由两个轴相乘得出。切勿只使用其中一个。
维度 1 — 表面关键性:这段代码控制什么?
| 级别 | 范围 | 原因 |
|---|---|---|
| P0 — 严重 | 资金(定价、套餐、配额、额度、计量、开票、退款、支付服务商)、身份验证/授权/会话/令牌、租户隔离、密钥与密钥处理、数据删除或破坏性迁移、已发布的 SDK/客户端包接口、公共 API 请求/响应契约、部署/基础设施/生产配置 | 出错会造成资金损失、数据泄露,或一次性破坏所有使用方。通常无法仅通过回滚代码来恢复。 |
| P1 — 高 | 核心领域逻辑和共享库(大多数功能依赖的模块)、Webhook、后台作业与队列、缓存与失效处理、速率限制、重试/幂等性、外部服务商集成、数据库架构(非破坏性)、影响事故响应的可观测性、CI/发布流水线 | 影响范围广或存在静默故障模式;通常可以回滚,但损害已经扩散。 |
| P2 — 中 | 功能局部业务逻辑、尚无调用方的新端点、内部管理工具、非核心 UI 流程、状态管理、非关键路径上的验证、开发工具 | 影响范围有限,故障会明显暴露,且易于回滚。 |
| P3 — 低 | 文档、注释、文案、样式、仅测试变更、内部脚本、由经过验证的生成器生成的快照、仅用于开发工具的依赖升级 | 不影响用户的运行时行为。 |
根据代码的作用来分配范围级别,而不只是看代码所在的位置。名为 utils/ 的路径
如果用于计算价格,就是 P0。billing/ 中的文件如果只导出一个用于仪表盘标签的
TypeScript 类型,则不是 P0。
应让你仔细检查的路径和标识符信号——pricing、plan、tier、 quota、credit、invoice、charge、refund、subscription、meter、usage、stripe、payout、 auth、session、token、permission、role、tenant、org、secret、key、migration、drop、 delete、truncate、sdk、client、public、v1/v2、webhook、cron、worker、deploy、env。
完整的信号表、CarsXE 单体仓库的各领域映射,以及如何读取仓库级覆盖文件:references/criticality.md。
轴 2 — 变更类别:这个 diff 实际上对该表面做了什么?
| 类别 | 示例 | 对等级的影响 |
|---|---|---|
| 契约变更 | 更改请求/响应结构、重命名或移除导出项、更改状态码、更改数据库列语义、更改事件载荷、更改默认值 | 保持表面等级;如果使用方是外部的或未进行版本管理,则提升 1 级 |
| 行为变更 | 更改逻辑、更改条件、更改顺序、在实时路径上新增分支 | 保持表面等级 |
| 增量变更 | 新代码路径由标志控制或新增端点,但目前尚无调用方使用;新增可选字段 | 降低 1 级,但前提是你已确认没有实时路径会触达它 |
| 配置/基础设施 | 环境变量、功能标志默认值、超时、限制、IAM/权限、流水线步骤 | 保持表面等级;P0 表面上的环境变量/权限变更仍为 P0 |
| 非行为变更 | 注释、文档、格式调整、更新所有引用后的纯重命名、仅涉及测试 | 降至 P3,但前提是确认它确实不会改变任何行为 |
| 依赖项 | 添加、升级或移除软件包 | 如果在生产环境中运行,最低为 P1;如果涉及身份验证、加密或支付,则为 P0 |
**有效等级 = PR 中任一单独代码块的最高有效等级。**绝不取平均值。一个 900 行的 P3 文档 PR,只要还改动了一个定价常量,就是 P0 PR。
然后应用以下修正项:
- +1 级,如果回滚无法恢复已经发生且用户可见的状态:数据迁移、回填、删除、Stripe/服务提供商对象变更、已发布的软件包版本,或已发送的邮件。尚未发布的工作流配置、模板或延迟仍可回滚——不要因此升 1 级。
- +1 级,如果你发现所改代码曾多次修复失败(步骤 2.7)。跳过历史检查不应升 1 级。
- −1 级,如果变更完全由默认关闭的标志控制,_并且_你已确认该标志保护了每一条新增路径。
记录等级的格式如下:P1 (surface: shared vehicle-history cache, class: behavior change, modifier: +0)。用一行说明你的判断依据——作者必须能够对此提出异议。
防止通胀的规则
分级的目的是集中精力,而不是让所有事情都变得吓人。
- 不要只根据文件夹名称分级。打开代码块查看。
- 不要根据 PR 大小分级。40 个文件都只是文案修改,也属于 P3。
- 不要把“修改了一个导入支付模块的文件”视为 P0。追踪修改的代码行是否真的会影响资金。
- 不要因为某个类别 可能 适用就提高等级。指出具体是哪一行使其适用。
- 面向用户 ≠ P0/P1。醒目、可回滚的 UI 和文案属于 P2。只有当修改的代码行会计算或授权资金/访问权限,或更改公开契约时,才提高等级。
- 任务、队列和邮件工作流并不自动需要人工把关。根据审查难度,它们仍属于 P1;如果修改后的行为经过测试或观察验证,则可以自动合并。
- 如果确实无法判断某个部分的作用范围,请说明这一点,并按最佳判断分级。打开代码块后,如果代码行无法造成最坏结果,就降低等级。未经验证、可能静默造成高昂代价的代码块仍需交由人工审查;未经验证的邮件延迟则不需要。
步骤 2.2 — ClickUp 工单对应关系(条件适用)
仅当 PR 标题匹配 \[CU-([A-Za-z0-9-]+)\](例如
[CU-1234] Cache plate lookups)时,才执行此步骤。如果标题没有此类标记,或没有 PR
标题,则跳过此步骤:不要调用 ClickUp,也不要在正文、分支或提交中查找工单 ID。
审查照常进行。
存在此标记时:
- 使用
clickup_get_task读取工单,并设置include: ["description"]。先尝试将CU-<token>作为自定义 ID,再尝试单独使用<token>。 - 将工单的核心诉求提炼为 1–3 条具体行为描述。
- 将每条诉求对应到修改的代码块。判断行为,而不是措辞或文件布局。
- 记录结论:符合、部分符合、不符或未验证。
不符的差异(实现了与工单诉求不同的功能)或缺少工单核心诉求的 部分符合差异,均属于重大问题,违背 PR 声明的目的 → Gate 1,BLOCKED。措辞含糊但存在合理解读的工单应作为问题提出,绝不判为不符。无法访问或内容为空的工单只需备注,不构成关卡。此检查为只读:绝不评论或更新工单。
查找顺序、判定标准、边缘情况和输出格式:references/clickup-ticket.md。
步骤 2.5 — 判断大型改动前先了解代码库
对于大型 PR、不熟悉的仓库,或涉及共享实用工具的改动,不要一开始就撰写问题发现。先构建一份轻量级依赖关系图,让审查基于代码的实际工作方式。
大型仓库审查流程
只要 PR 更改了约 6 个以上文件、涉及共享代码,或更改了其他层使用的契约,就使用此流程。
-
找到入口点
- UI:页面、路由、屏幕、表单、钩子、处理程序。
- API:路由处理程序、服务端操作、控制器、Webhook、作业。
- 库/包:导出函数、包导出项、公共组件、SDK 客户端。
- 数据库/领域:变更、存储库、架构、迁移、验证器。
-
从已更改代码向外追踪
- 对于每个已更改的导出函数/组件/类型,查找当前调用方。
- 对于每个已更改的路由/事件/作业,查找客户端、测试、文档、架构和生成的类型。
- 对于每个已更改的工具函数,至少检查 2–4 个有代表性的调用点,尤其是高风险调用点。
- 对于每个已删除/重命名的符号,确认导入、配置引用、文档和测试均已更新。
-
审查前按作用分类文件
- **事实来源:**架构、验证器、迁移、领域模型、API 契约、权限映射。
- **适配器:**提供商客户端、数据库存储库、Webhook 解析器、序列化/映射层。
- **编排器:**服务、变更、作业、用例函数、路由处理程序。
- **呈现层:**组件、页面、屏幕、表单、文案、样式。
- **工具函数:**共享纯函数辅助工具、格式化器、解析器、钩子、包导出项。
- **测试/支持:**测试夹具、模拟对象、工厂函数、快照、CI 配置。
-
按依赖顺序检查契约是否一致
- 事实来源 → 适配器 → 编排器 → 呈现层 → 测试。
- 如果较低层更改了返回值结构,确保所有上层仍能正确处理。
- 如果 UI 更改了提交的负载,确保 API 验证器和服务端行为与之匹配。
- 如果验证器发生变化,确保测试夹具、客户端和文档均已更新。
-
查找现有的本地模式
- 在指出某处不一致之前,先搜索附近的文件。
- 除非现有模式不安全,否则优先遵循仓库既定模式,而不是通用最佳实践。
- 指出一致性问题时,明确说明所指的模式:“
apps/api/routes/*中的其他路由会先使用requireOrgMember(),再读取租户数据。”
-
按影响范围从大到小审查
- 公共契约、身份验证、数据写入、迁移、支付、Webhook。
- 共享工具函数/钩子/包。
- 特定功能逻辑。
- UI 润色和命名。
-
证据不足时停止
- 不要仅凭文件名推断行为。
- 未检查调用点前,不要假定某个工具函数有误。
- 如果缺少上下文,请在“问题/假设”下指出具体缺少的上下文,而不是凭空提出问题。
工具函数/共享代码审查流程
共享工具函数很危险,因为细微的行为变化可能会悄无声息地影响许多功能。审查这类代码时,应采用不同于功能局部代码的方式。
对于每个已更改的工具函数/钩子/共享组件/包导出项:
- 确定它属于公共 API、内部辅助工具,还是单一功能的辅助工具。
- 如果调用点较少,就全部检查;如果很多,则抽样检查有代表性的调用点。
- 对比变更前后在边界情况下的行为:
null、undefined、空字符串、空数组、零、无效日期、环境变量缺失、Promise 被拒绝、重复调用。 - 检查现有测试是否描述了旧行为。如果 PR 更改了预期行为,应明确更新测试。
- 检查名称是否仍符合行为。名为
isValidX的辅助工具不应开始修改数据、获取数据、记录日志或意外抛出错误。 - 留意隐式契约变更:同步 → 异步、抛出错误 → 返回 null、可变 → 不可变、顺序稳定 → 顺序任意、精确匹配 → 模糊匹配。
只有在能够将共享代码的行为变更与实际调用方或公共契约关联起来时,才报告相关问题。否则,应提出有针对性的问题。
关联部分检查清单
当 PR 涉及某个流程的一部分时,快速检查相邻部分:
| 更改部分 | 还需检查 | 常见遗漏 |
|---|---|---|
| UI 表单 | 服务端操作/路由、架构、默认值、提交禁用状态 | 负载不匹配、重复提交、错误丢失 |
| API 路由 | 身份验证守卫、验证器、服务、响应类型、前端调用方 | 缺少服务端身份验证、错误封装不一致 |
| DB 架构/迁移 | 仓库、旧数据、索引、回滚/默认值 | 生产环境中的旧记录导致故障、查询缓慢 |
| Webhook/作业 | 签名/幂等/重试逻辑、日志、副作用 | 重复履约、重放错误 |
| 共享工具 | 调用方、测试、导出的包接口 | 功能间出现隐性行为变更 |
| 查询/缓存变更 | 查询键、失效、分页、乐观更新 | 数据过时/跨用户泄漏 |
| 配置/环境变量变更 | CI、预发布/生产环境名称、文档、回退行为 | 仅在本地正常运行 |
| 依赖升级 | 锁文件、对等依赖、构建输出、运行时兼容性 | CI/构建或服务端/客户端边界出现问题 |
超大型 PR 的上下文预算策略
如果 PR 太大,无法深入检查每一行:
- 检查 diffstat,并找出风险最高的 20% 文件。
- 优先完整审查高风险文件和公共契约。
- 仅在验证生成器或模式无误后,再抽查重复的机械性变更。
- 标注“审查覆盖范围:部分”,并说明哪些内容未经过深入审查。
- 如果高风险 PR 的审查覆盖范围不完整,绝不要给出 5/5。
步骤 2.6 — 评审前收集仓库信号
仓库已经记录了各项内容的负责人,以及团队关注的事项。与其猜测,不如先阅读仓库。这项检查成本低、价值高,还能让分级和评审人员分派更具体。
按以下顺序查找,仅检查实际存在的项目:
| 信号 | 位置 | 用途 |
|---|---|---|
| 负责人 | CODEOWNERS、.github/CODEOWNERS | 如果需要人工评审,注明应由谁评审;只有分支保护要求该审批时,才将其视为门槛 |
| 团队规则 | CLAUDE.md、AGENTS.md、.cursor/rules/、.greptile/、CONTRIBUTING.md | 项目特定约定;其优先级高于通用最佳实践 |
| 受保护路径 | 分支保护说明、.github/workflows/*、必需检查 | 哪些路径已要求审批——绝不要与比你的门槛更严格的仓库策略相矛盾 |
| 发布范围 | package.json 的导出项/files、changesets、openapi.*、*.proto、带版本号的路由目录 | 判断更改是否会对外发布 |
| 测试/CI 现状 | CI 配置、测试脚本、覆盖率配置 | 判断这里是否真的强制要求“有测试” |
| 危险注释 | // DANGER、// do not change、@internal、@deprecated、过往的 TODO(incident) | 可能表明这处改动忽略了团队积累的经验 |
如果 CODEOWNERS 与某个已更改路径匹配,且负责人不是作者,那么在安排人工评审时注明该负责人。只有分支保护或必需评审确实要求该审批时,CODEOWNERS 才是门槛。仅供参考的 CODEOWNERS 用于分派评审,而非否决更改。
步骤 2.7 — 以往尝试的取证
这是这项技能能执行的价值最高的一项检查。 对已经“修复”过两三次的代码再次进行修改,并非普通改动。要么先前的修复只弥补了表象,这次也会如此;要么这段代码路径存在尚无人理解的特性。这两种情况都意味着要深入调查。只有实际发现回滚、热修复或反复修复链时,才需要人工评审。
当 PR 为 P0/P1、修复缺陷,或涉及明显经历过频繁改动的文件时,都要执行此检查。仅当没有可检查的仓库时才跳过。
核心问题
- 这个确切的缺陷或代码路径以前修复过吗?
- 之前的修复是否被回滚、热修复,或紧接着又进行了另一次修复?
- 之前的尝试遗漏了什么?这个 PR 是在解决遗漏,还是在重复之前的做法?
- 这是根因修复,还是在另一个调用点重复修补相同的表象?
- 这个文件是否是频繁变更的热点(近期有许多作者多次修改)?
命令指南
# Who last touched the changed lines, and in what commit
git blame -L <start>,<end> -- <file>
git show <sha> --stat # then read the message and linked PR/issue
# Every change to this file, newest first — look for clusters and repeat authors
git log --oneline --follow -20 -- <file>
# When did this specific logic/symbol/string appear or change? (pickaxe)
git log -S '<symbol_or_constant>' --oneline -- <path>
git log -G '<regex>' --oneline -- <path>
# Failed-fix fingerprints
git log --oneline -i -E --grep='revert|rollback|hotfix|re-fix|fix again|regression|incident|p0|p1' -- <path>
# Churn hotspot: how volatile is this file lately?
git log --since='90 days ago' --oneline -- <file> | wc -l
git log --since='90 days ago' --format='%an' -- <file> | sort -u | wc -l
# Repo-wide hotspot ranking
git log --since='6 months ago' --name-only --format='' | sort | uniq -c | sort -rn | head -20
如果可以访问 GitHub,请顺着讨论查找相关人员的上下文:
gh pr list --state all --search '<keyword or file path>' --limit 20
gh pr view <n> --comments # what reviewers worried about last time
gh issue list --state all --search '<symptom>' --limit 20
历史记录如何影响审查
- 先前的修复在几天内被撤销或紧急修补 → 提高 1 个严重性等级,并设置人工审查门槛。说明涉及哪个提交,以及发生了什么。只有在找到这条关联链时才触发。
- 同一行为的第三次或后续尝试 → 将“此修复是否解决了根本原因?”作为阻断性问题,而不是小问题。引用先前的尝试,并指出它遗漏了什么。
- 过去 PR 中的审查意见指出了本 PR 重新引入的问题 → 高置信度发现;引用旧 PR。
- 变更频繁的热点(90 天内 >8 次变更,或 4 位及以上不同作者) → 提高审查力度并注明,但仅凭这一点不构成门槛。
- 历史记录干净 → 用一行说明。这是支持自行合并的真实证据,应如实报告。
- 未检查历史记录 → 写明“历史记录:未检查(无法访问仓库)”。这只是备注,不是门槛。
绝不要捏造历史记录。如果没有运行这些命令,不要暗示自己运行过。
更深入的指南,包括如何重建撤销链,以及如何解读 Bug 的修复沿革:references/history-forensics.md。
步骤 3 — 分轮检查
分轮进行审查,避免能力较弱的模型遗漏跨文件问题。
第 A 轮 — 意图与回归
请检查:
- 实现是否确实符合 PR 标题/描述?
- 如果标题中包含
[CU-<id>],差异是否实现了 ClickUp 工单的要求(步骤 2.2)? - 是否意外移除了现有行为?
- 旧调用方、客户端、查询参数、响应结构、环境变量名称或数据格式是否仍然兼容?
- 如果行为发生变化,是否有迁移或发布计划?
第 B 轮 — 正确性与契约
请检查:
- 是否处理了 null/空值/错误/加载状态?
- 异步操作是否经过等待,或明确采用了“即发即弃”方式?
- 用户双击、刷新、重试或加载更多时,是否可能出现竞态条件?
- 验证器、类型、运行时模式与数据库约束是否保持一致?
- 前端与后端的 API 请求/响应结构是否一致?
- 缓存键和失效规则是否正确?
- 分页、排序、筛选、时区、区域设置和边界情况是否正确?
第 C 轮 — 安全与隐私
请检查:
- 每个信任边界是否都在服务端进行验证?
- 是否在服务端执行授权检查,而不仅仅在 UI 中检查?
- 用户能否通过更改 ID 访问其他租户/用户/组织/资源的数据?
- 日志或响应中是否会泄露密钥、令牌、API 密钥、Cookie、标头或提供方错误?
- 是否可能发生 SQL/NoSQL/Shell/路径/HTML/模板注入?
- Webhook 是否经过验证并防止重放?
- 重定向、CORS、CSRF、Cookie 标志和速率限制是否适合该端点类型?
第 D 轮 — 数据、迁移与运维
请检查:
- 架构变更是否包含安全的迁移/回填/默认值?
- 新增的查询模式是否添加了索引?
- PR 是否保留了现有数据,并能处理不完整/旧记录?
- 在需要时,重试是否具备幂等性?
- 后台作业是否易于监控,且可安全地重新运行?
- 在现有环境变量和权限下,代码能否在预发布/生产环境中正常运行?
第 E 轮 — 测试
请检查:
- 新增或修改的业务逻辑是否有有意义的测试?
- 测试是否覆盖边界情况和故障模式,而不只是正常流程?
- 测试是否确定:不访问真实网络、不使用真实计时器、不依赖执行顺序,也不会泄漏共享状态?
- 快照变更是否有真实原因?
- Mock 是否过于宽泛,以致掩盖了契约破坏?
第 F 轮 — 可维护性与一致性
请检查:
- 是否遵循项目中相近代码的文件布局、命名、错误处理、响应、状态、导入、测试和抽象模式?
- 是否重复实现了现有 util/hook/service/mutation 中的逻辑?
- 代码是否更易于下个月修改,还是引入了并行系统?
- 抽象是否为时过早,或对当前用例来说过于通用?
- 是否有写得巧妙但难以理解的单行代码?
只有在能引用或指出一个具体的现有模式时,才指出一致性问题。
Pass G — 简洁性、复用与“马尾辫式”过度设计检查
使用此检查来发现这类情况:PR 构建了自定义系统,而代码库、标准库、平台、数据库、框架或已安装的依赖其实已经能解决问题。这不是风格检查,而是降低风险的检查:自定义代码越少,漏洞越少,边界情况越少,也越容易维护。
在接受新的抽象、自定义状态机、重试循环、缓存、格式化器、验证器、调度器、包装器、工厂或类似框架的辅助工具之前,先运行简洁性阶梯。从上到下检查,并在第一个确实满足需求的层级停下:
- 这确实有必要吗? 如果需求只是推测,只有在新增代码造成实际维护成本或风险时,才标记为 YAGNI。
- 代码库中已经有了吗? 搜索现有的辅助工具、钩子、服务、类型、模式、映射器、服务提供方客户端、查询键工厂、重试辅助工具、日志记录器或错误封装。
- 标准库能做到吗? 优先使用内置的日期、URL、数组、对象、Promise、路径和加密辅助工具,而不是自定义实现。
- 平台原生功能能做到吗? 优先使用 HTML 输入控件、CSS、浏览器 API、数据库约束和索引、HTTP 语义及框架原语,而不是手工实现。
- 已安装的依赖能做到吗? 如果 React Query/TanStack Query、Zod、date-fns、lodash、路由工具、ORM 辅助工具、队列/任务库等已经安装且适用,就复用它们。
- 能否直接表达相同行为? 优先使用简单明了的本地函数,而不是通用类、工厂或配置层。
- 只有上述方法都不适用时,才接受自定义代码,并且代码规模只需满足实际需求即可。
哪些过度设计问题值得报告
满足以下所有条件时,才报告:
- PR 新增了有实质意义的自定义代码或抽象。
- 仓库、平台、标准库、框架、数据库或已安装的依赖中存在更简单的方案。
- 更简单的方案能满足实际需求,而不只是实现一个玩具版本。
- 自定义实现带来了具体风险:行为重复、遗漏边界情况、缓存不一致、重试漏洞、状态过期、测试更难、增加打包或运行时开销,或造成未来的维护负担。
- 你能清楚说明替代方案。
以下情况不要报告:
- 自定义代码是因为依赖或框架无法满足实际需求。
- 更简单的方案会掩盖重要的领域逻辑。
- 该抽象已成为项目中稳定的模式,并有多个真实调用方。
- 此改动有意保持局部且临时,并且范围上限明确。
- 你只是偏好另一种风格,却说不出具体风险。
常见的「重复造轮子」模式
| 重复造的代码 | 优先采用 | 重要原因 |
|---|---|---|
| 手动处理 fetch 状态、重试、去重、缓存、重新获取、分页 | 仓库中已有的 TanStack Query / React Query | 自定义实现通常会遗漏取消请求、过期数据、去重、重试策略、缓存失效和竞态处理 |
使用 useCallback/useEffect 链处理派生数据 | 在渲染时计算 / 仅在计算开销较大时使用 useMemo | 避免状态过期、额外渲染和依赖项错误 |
| 自定义表单验证,且客户端和服务器端重复实现 | 已在使用的共享 schema/runtime 验证器 | 防止请求负载不一致和错误信息不一致 |
| 自定义日期/URL/查询字符串解析 | URL、URLSearchParams、Intl、现有日期工具 | 避免编码/时区/区域设置相关的边界问题 |
| 仅在应用层检查唯一性 | DB 唯一约束/索引 + 友好错误 | 防止竞态和重复数据 |
| 自定义轮询/重试循环 | 框架/查询/任务重试原语 | 防止循环失控、重复工作和退避策略不一致 |
| 为单一实现新增封装 | 直接调用函数/组件 | 避免虚假的灵活性和额外的间接层 |
| 为单个值新增通用配置 | 在使用处附近内联常量 | 避免「为未来可能的需求设计架构」 |
| 为一个小型辅助函数新增依赖 | 标准库或现有依赖 | 降低打包、审计、安装和维护成本 |
| 重复实现响应/错误映射 | 现有 API 信封/辅助函数 | 防止客户端契约不一致 |
简化问题的严重程度
- major:自定义代码可能导致实际的错误行为、数据/安全/支付风险、重复网络请求、过期/跨用户缓存、重试失效或公共 API 不一致。
- minor:自定义代码目前可以正常工作,但会显著增加维护成本、重复仓库中现有的模式,或很可能导致未来的改动出现偏差。
- nit:仅是细微的可读性简化,没有实质性风险。不对 nit 评分。
简化问题的必需表述方式
简化问题必须指出更简单的方案,以及当前代码中的具体风险。
不佳:「过度设计了。使用 React Query。」
良好:「这个 hook 重新实现了查询状态、重试和请求去重,尽管本仓库已经使用 TanStack Query 读取 API。当两个组件同时挂载时,两者都会调用 fetchInvoices() 并争相设置本地状态;使用 useQuery({ queryKey: ['invoices', orgId], queryFn }) 就能实现去重、集中管理重试策略,并与现有的缓存失效流程保持一致。」
阶段 H — 故障影响分析:这可能导致什么无法正常工作?
问题发现会说“这行代码有误”。故障影响分析则会说“如果这里有误,哪些功能会停止工作”。这是两种不同的产出,作者两者都需要。对每个 P0/P1 PR,以及任何会改变共享行为的 P2 PR,都运行此阶段。
对于每项行为或契约变更,逐层向外分析:
- 谁会使用它? 先看直接调用方,再看它们的调用方,直到到达面向用户或可从外部观察到的边界。优先搜索导入语句和调用位置,而不是根据名称推断。
- 故障模式是什么? 明显故障(抛出异常、返回 500、测试失败),还是静默故障(数值错误、缓存过时、记录缺失、舍入方式略有不同)?P0 路径上的静默故障是最需要关注的问题。
- 谁会察觉,何时察觉? 部署后立即察觉、下一个计费周期、月底对账时,还是只有客户投诉后才察觉?
- 什么机制能检测到它? 指出具体的测试、告警、日志、仪表盘或健康检查。“目前没有任何机制能检测到”本身就是值得报告的问题。
- 如何撤销影响? 回滚代码、关闭开关、回滚迁移、手动修复数据,还是在服务提供方一侧清理?
只报告能在代码中找到依据的场景。两个追踪充分的场景,胜过八个臆想的场景。格式如下:
| 可能出什么问题 | 触发条件 | 受影响对象 | 检测方式 | 回滚方式 |
|---|---|---|---|---|
| 超额发票会重复计算用量 | 部署后,任何账户的用量超过套餐限额 | 使用按量计费套餐的付费客户 | 无 — 没有针对发票金额变动的告警 | 回滚代码 + 重新开具受影响的发票 |
如果某一行在静默或代价高昂的故障路径上写着“检测方式:无”和“回滚方式:手动修复数据”,就必须要求增加观测手段(例如断言该值的测试、一次实际运行,或流程截图)。这是验证要求。如果已经观察到该行为,就不需要再由人工额外签字确认。
还要说明在边界确实明确时,此 PR 不会导致什么无法正常工作——“变更仅限于管理端导出页面;没有改动共享模块、契约或持久化数据。”这句话正是获得自行合并资格的依据。
第一阶段 — 观察:能否看到它实际运行?
对于每项行为变更,请判断它是已观察还是仅仅经过推理,并标注出来。在静默或高成本路径(资金、权益、授权、数据删除、破坏性公共契约)上,仅经推理的行为属于未知情况,应交由人工处理。P2 级文案、UI 或工作流时序方面仅经推理的行为应作为备注,而非阻塞条件。
证据按强度从高到低排序:
- 直接观察 — 已调用端点并读取响应;已运行脚本;已执行测试并查看其输出;截图展示了该状态。
- 现有自动化检查 — 差异中有一项测试,能够确实覆盖变更路径;通过阅读断言而非测试名称确认这一点。
- 静态验证 — 类型、架构对齐、穷尽匹配,或能在出错时失败的编译器或验证器。
- 追踪推理 — 你沿着代码追踪,判断它应该可以正常工作。
- 作者的说法 — 单凭这一点没有价值。
在环境中能运行的内容,就运行它。无法运行时,请提供一份观察计划,让作者或审阅者能在一分钟内执行:
**Observe it:** `curl -s localhost:3000/api/v1/plate/ABC123 | jq .confidence`
Expect a number between 0 and 1; before this change the field was absent.
规则:
- 断言函数已被调用的测试不算观察。断言了_值_的测试才算。
- UI 截图是优先采用的证据,但不是阻塞条件。对于结账、登录或授权流程,未经验证的视觉行为属于静默或高成本路径上的未知情况。对于其他 UI,即使覆盖完整且测试通过,也仍可自行合并;请注明视觉行为尚未验证。
- 迁移应使用真实数据形态的副本进行观察,而非空的开发数据库。
- 如果 PR 中没有任何内容能在不搭建完整手动环境的情况下进行观察,那么这本身就值得指出——这通常意味着应拆分变更。
PR 过大而无法观察时:建议拆分
如果差异太大,以至于你无法对每个代码块形成判断,请不要只做部分审查并据此设置阻塞条件。应建议拆分,并具体说明拆分边界:
🔀 建议拆分。 1,400 行代码分布在 31 个文件中,混合了架构迁移、新的提供商客户端和仪表板接线。按迁移、带独立测试的客户端、UI 这一顺序分层后,每一层都能独立运行,并且每一层落地时,其下方的行为都已验证。作为一个 PR,迁移中的错误只有在所有内容合并后才会显现。
指导:只有一个关注点且变更行数大约不超过 400 行的 PR,通常可以端到端观察。超过这一规模后,审查质量会下降,部分覆盖也会开始阻塞原本没有问题的 PR。拆分不代表拒绝——请明确说明这一点。
审查分类
在审查发现中使用以下类别。
正确性与回归
逻辑错误、流程中断、缺少边缘情况、不兼容的行为变更、条件错误、状态过期、分页/筛选/排序错误、时区/区域设置错误、重复提交问题、竞态条件。
安全与隐私
认证/授权漏洞、IDOR、注入、不安全的重定向、CSRF/CORS/Cookie 错误、机密泄露、不安全的日志、缺少 Webhook 验证、敏感数据暴露、易遭滥用的端点缺少速率限制。
数据与迁移
不安全的架构变更、缺少默认值/回填、缺少索引、数据丢失、非幂等任务、旧记录兼容性损坏、事务/原子性问题。
API 与契约
请求/响应结构不匹配、状态码错误、缺少运行时验证、导出 API 的破坏性变更、错误信封不一致、未版本化的契约变更。
错误处理与可观测性
吞掉错误、误导用户的错误信息、泄露内部信息、缺少清理操作、重要失败路径缺少日志/指标/追踪、无助的重试。
测试
变更的业务逻辑缺少测试、断言实现细节的弱测试、不稳定的测试、掩盖真实集成契约的 Mock、修复缺陷后缺少回归测试。
性能与可扩展性
N+1 查询、热路径上的高开销操作、不必要的序列化、无界循环、内存泄漏、过度轮询、糟糕的缓存失效处理、渲染风暴。只有在问题很可能造成影响时才指出。
可维护性与简洁性
函数过于复杂、嵌套过深、名称不明确、魔法值、无用或注释掉的代码、布尔参数陷阱、过早抽象、职责过多。
过度工程化与复用
自定义代码重复实现代码库、标准库、平台、数据库、框架或已安装依赖中的功能;不必要的包装器/工厂/类/配置层;在已有原语更安全的情况下,手动实现获取/缓存/重试/表单/日期/查询字符串逻辑。
DRY 与复用
复制粘贴逻辑、重复的常量、重新实现已有工具函数、并行数据结构、多个事实来源。
依赖与构建
不必要的依赖、有风险的版本变更、对等依赖冲突、tree-shaking 问题、循环导入、打包/服务端与客户端边界问题、包导出故障、CI/构建配置回归。
无障碍与 UX
缺少标签/角色/键盘支持、焦点状态异常、错误消息无法访问、加载/空白/错误状态不佳、布局破坏重要的用户流程。主要用于 UI PR。
代码库一致性
违反既有的本地模式,例如命名、文件结构、状态管理、响应格式、错误处理、导入、测试或领域边界。
严重性评估
critical
导致生产环境故障、安全/隐私漏洞、数据丢失/损坏风险、支付/身份验证绕过,或可能影响真实用户的迁移/部署问题。阻止合并。
major
实际行为风险、对变更的业务逻辑缺少重要测试、显著的可维护性负担、契约不匹配、明显违反 DRY,或极可能出现的错误。应在此 PR 中修复。
minor
影响范围较小的实际问题、有限的边缘情况、轻微的一致性偏差,或会拖慢后续工作的可读性问题。尽快修复,但不一定阻止合并。
nit
纯风格、命名润色、细微的可读性偏好,或可选的清理工作。绝不阻止合并。
升级规则
- 在身份验证、支付、数据删除、迁移、租户隔离、机密信息和公共 API 领域,小错误可能造成严重后果。仅在影响明确时升级严重性。
- 不要将在敏感领域发现的每个 major 问题都自动升级为 critical。这会造成误报。
- 对内部脚本/原型,除非架构/风格问题影响安全或重复使用,否则应降低其严重性。
- 对生成的代码,应审查生成器/配置,而不是生成的输出;除非生成的输出直接作为事实来源提交。
将每项发现归入以下三个类别之一
严重程度说明问题有多严重。分类说明接下来要做什么。为每项发现添加标签:
| 类别 | 含义 | 处理方式 |
|---|---|---|
| 可操作项 | 可以明确指出缺陷和修复方案 | 列入 Issues,并按严重程度排序 |
| 小问题 | 确实存在,但可选;没有明确的风险 | 列入 Nits,最多 5 项,绝不影响评分或门槛 |
| 不明确项 | 可能是缺陷;取决于你不了解的背景(产品意图、未写明的约束,或在其他地方做出的决定) | 列入 Questions。只有在回答会影响安全性时,才触发人工审核——例如金额、权益或访问权限,而 PR 从未说明其意图 |
不明确项这一类别能让另外两类保持客观。没有它,不确定的发现就会被归为误报,或干脆不提。指出具体缺失的背景——“旧费率是否应适用于正在进行的计费周期?”——而不是“请仔细检查”。
避免误报
不要报告:
- 仅与 Linter/格式化工具有关的问题,除非它们掩盖了真实错误。
- 缺乏项目证据的个人偏好。
- 没有明确热点路径、规模条件或明显浪费作为依据的推测性性能问题。
- 并非由 PR 引起的既有无关问题。
- 针对简单胶水代码、纯文案改动或极其简单的 UI 连线遗漏测试。
- 没有说明维护成本的“可以写得更整洁”。
- 仅仅因为代码出现两次就添加新的抽象;在行为稳定之前,重复代码可以接受。
- 如果依赖已经安装且以类似方式使用,则不要提出依赖方面的担忧。
- 无助于提升安全性或可读性的类型小问题。
不确定时,提出问题,而不是报告发现。
评分标准
从 5 分开始。先扣分,再应用硬性上限。
扣分项
| 扣分 | 条件 |
|---|---|
| −3 | 每项严重问题 |
| −1 | 每项重大问题,最多累计扣 2 分 |
| −0.5 | 有 3 项或更多轻微问题 |
| −0.5 | 对已更改业务逻辑的测试有实质性缺失 |
| −0.5 | 存在显著的 DRY/复用问题或过度设计 |
| −0.5 | 存在 2 项或更多代码库一致性问题 |
小问题不影响评分。
硬性上限
- 存在任何严重问题:最高 2/5。
- 存在 2 项或更多严重问题:最高 1/5。
- 存在任何未受保护的身份验证/管理员/支付/数据删除路由:最高 1/5。
- 存在任何会破坏 PR 主要目的的重大问题:最高 2/5。
- 高风险 PR 没有针对已更改业务逻辑的测试:最高 3/5,即使代码看起来正确。
- 只有小问题/轻微风格问题:最低 4/5。
- 未发现有实质影响的问题:5/5。
扣分后向下取整,除非适用硬性上限。最低分为 1 分。
评分含义
| 评分 | 标签 | 结论 |
|---|---|---|
| 5/5 | 优秀 | ✅ 可以合并 |
| 4/5 | 良好 | ✅ 可以合并,可选择进行润色 |
| 3/5 | 可接受 | ❌ 修复列出的问题后再合并 |
| 2/5 | 需要改进 | ❌ 请求修改 |
| 1/5 | 尚未就绪 | ❌ 请求修改 |
评分针对的是代码质量,不是合并决定。 如果 PR 说明了新的金额,并且有测试或运行结果加以验证,那么 5/5 的定价变更可以由作者自行合并。即使是 5/5 的定价变更,如果意图没有解释清楚,仍需要人工审核。先计算评分,再独立运行下方的门禁。
第 4 步 — 合并决定门禁
准确输出三个结论之一。按顺序运行各道门禁来做决定——遇到第一个符合条件的结论时立即停止。不要先得出结论,再倒推理由。
| 结论 | 含义 |
|---|---|
| 🛑 BLOCKED | 不要合并。差异中存在错误或无法验证之处。 |
| 👥 HUMAN REVIEW REQUIRED | 代码可能没有问题,但必须由另一人批准。务必说明由谁审核以及要检查什么。 |
| ✅ SELF-MERGE OK | CI 通过后,作者可以在无需其他审核者的情况下合并。 |
门禁不变量
无论差异内容如何,也无论作者怎么说,以下规则始终适用:
- 对未知项保持谨慎,而不是对涉及的文件或领域。 未经验证的金额、权益、授权、数据删除或破坏性公共契约变更应交由人工审核。已验证的 P0 变更若没有发现问题,则无需人工审核。
- 仅对必需输入采取失败关闭策略。 差异不可读、CI 状态未知或存在合并冲突时,应升级处理。跳过历史记录、未命名的 CODEOWNER 或消费者搜索不完整,不构成升级理由。
- 先查看代码块,再决定是否可以降低审核级别。 文件路径、目录以及“此文件靠近计费代码”都是查看的理由,但不能作为拒绝合并的依据。
- 绝不代替他人建议合并。 结论只是给作者的建议;批准、请求修改和合并都是人工操作。
门禁 0 — 机械性预检查(审核可靠性)
这些检查简单、确定,应优先运行。检查失败意味着仅凭快速浏览代码无法有把握地建议自行合并——建议拆分变更,或将结论设为以 CI 通过为条件;不要仅仅因为文件路径看起来令人担忧就要求人工审核:
- PR 没有合并冲突,并且相对于基准分支足够更新,足以进行有效审核。
- 没有尚未解决的“请求修改”审核意见。
- CI 已通过,或者结论明确以 CI 通过为条件。
- 差异处于可观察范围内:约 ≤500 行变更且 ≤20 个文件;只有在变更属于机械性重复且已验证其模式时,才可超出此范围。
- 差异内容完整可读。
规模是审核可靠性的参考指标,而非风险指标。一行定价变更也属于 P0 级审查,需要更高的观察标准;但这并不意味着应因文件路径而拒绝。超出上述范围时,建议拆分变更,而不是对仅快速浏览过的代码给出有把握的结论。
门禁 1 — 阻断项(任一项符合 → BLOCKED)
- 存在任何未解决的严重问题。
- 存在任何重大问题,导致 PR 无法实现其声明的目的——包括差异内容与标题中的
[CU-<id>]工单(第 2.2 步)不符。 - 在读取或写入数据的路径上,缺少身份验证、授权或租户隔离检查。
- 提交或记录了密钥、令牌或凭据。
- 存在破坏性迁移或删除数据的代码,但没有经过验证的回滚或备份方案。
- 可以证明资金转移或价格计算有误。
- 差异内容实质上不可用——有人要求你批准你无法阅读的代码。
Gate 2 — 需要人工审核(满足任意一项 → 需要人工审核)
关注验证情况和未解决的风险,而非表面标签。P0/P1 本身不会触发此门槛:
- 静默或高成本路径上的行为未经验证 — 涉及资金转移、权限、授权/租户范围、数据删除或有破坏性变更的公开契约,且没有任何验证手段(没有断言该值/检查的测试、没有运行记录,也没有该流程的截图)。
- 存在未解决的重大问题,或存在会影响安全性的模糊产品问题(例如:“超额使用 3 美分是预期行为吗?”,但 PR 从未说明)。普通小问题和“请再检查一下”不算。
- 覆盖不完整 — 未完整阅读差异,或 PR 混杂过多内容而难以审核;应优先拆分,而非设置一个徒有其名的人工审核门槛。
- 涉及代码行存在已知问题历史 — 你实际发现了回滚/紧急修复/重复修复链(步骤 2.7)。跳过历史检查应作为备注,而非触发审核门槛。
- 仓库策略要求人工审核 — 分支保护或必需的 CODEOWNERS 批准。请注明具体策略。仅供参考的 CODEOWNERS 文件不算。
- 破坏性变更影响消费者未知的外部契约 — 重命名、移除、修改默认值或更改已发布 API/SDK 的类型。经确认确属可选的新增字段不算。
- 产生未经验证的不可逆副作用 — 群发邮件、修改 Stripe 对象、发布软件包、执行破坏性迁移。由于无法撤回而需要验证(内容、收件人、触发条件),但这不会自动触发人工审核。
如果均未触发且 Gate 1 无问题 → 继续 Gate 3,包括 P0,也包括你在本次会话中编写的代码(披露作者身份;不要自动升级审核)。
Gate 3 — 自行合并(必须全部满足)
P2/P3 不是前提条件。审核完成时,默认可以自行合并:
- Gate 0 的机械性预检全部通过(或者 PR 过大只是因为内容机械重复,且你已验证其模式)。
- 未触发任何 Gate 2 条件。
- 没有严重问题,也没有重大问题。只有次要问题和细枝末节。
- 审核覆盖完整 — 你阅读了每个变更代码块,而非抽样阅读。
- 已观察到静默或高成本的变更行为(阅读了测试断言、运行过,或查看了该流程的截图)。其他行为已通过观察、静态验证,或确认不涉及行为变更(文档、文案、注释、格式、仅测试、由未变更且已验证的生成器生成的输出)。
- 影响范围有限,且你能用一句话说明其边界。
- 没有未解决的安全性模糊问题(金额、权限或访问权限的意图从未在 PR 中说明)。
如果全部通过,请明确、简短地说明这一点 — 包括已验证的定价或授权调整、邮件工作流延迟,以及你观察过的由 Agent 编写的 P2/P0 代码。不要凭空制造犹豫的理由 — 过度谨慎的审核门槛会让作者学会忽视它。
撰写决策
始终按以下顺序包含:
- 结论。
- 实际等级,以及说明原因的一句话。
- 原因——触发的具体门槛,引用具体内容(“门槛 2.1:
packages/billing/src/plan-limits.ts修改了超额乘数,且没有测试断言新的美分数”),或在自合并时写“未触发任何门槛”。 - 谁应该审查——如果存在,指定一名 CODEOWNER;否则,具体点名
git blame和近期历史显示实际处理过这段代码的工程师;再否则,写明角色(“负责账单的人”“拥有生产数据库访问权限的人”)。绝不要只写“资深工程师”。自合并时省略。 - 审查者应重点关注什么——列出 1–3 项具体内容,不要写“审查这个 PR”。自合并时省略。
- 如何观察其是否正常工作——在变更可观察时,给出要运行的命令和预期输出。
- 预计需要多长时间——给出大致估时,表明你了解变更内容(“约 10 分钟,重点看两个文件”)。
- 如何解除门槛——说明通过什么具体验证或明确意图即可允许自合并(“添加测试,断言套餐边界上的超额乘数”)。绝不要写“这个范围总是要交给人工审查”。
第 8 点很重要:门槛应起到指导作用,而不只是阻拦。
门槛方面的诚实要求
- 此结论仅供参考,绝不凌驾于分支保护、必需审批或组织政策之上。如果仓库配置要求更严格,应遵从并明确说明。
- 绝不要建议通过自合并来跳过观察或绕过实际阻碍。紧急情况只影响审查速度,不影响是否需要审查未经验证的静默变更或高成本变更。经过验证的 P0 可以自合并。
- 作者提出异议时,绝不要弱化阻碍。重新审视证据;如果证据成立,结论就应维持不变。
- 如果跳过了历史或仓库信号,应在决策中说明——不要让静默跳过看起来像是没有问题,也不要把跳过操作视为触发门槛 2。
门槛判定示例,包括棘手的边缘情况:
references/merge-gate.md。
后续审查(修复后重新审查)
作者提交修复后,不要从头重新审查,也不要重新争论已解决的问题。按增量流程执行:
- 对比新状态与已审查状态(
git diff <old_sha>..<new_sha>)。 - 对每个先前发现的问题标记:已修复 / 部分修复 / 未处理 / 已修复但引入了新问题。根据代码核实——绝不要轻信“已完成”。
- 仅使用完整审查流程集审查新增代码块。
- 重新评估严重性:修复可能会提高等级(添加迁移的“小修复”现在属于 P1+)。
- 从头重新执行合并门槛判定。已被阻止的 PR 不会仅仅因为问题已修复就变成可自合并——新代码本身也必须通过门槛判定。
- 仅报告:问题解决状态、新发现、更新后的结论。保持简短。
当门槛判定结果为 SELF-MERGE OK 或 HUMAN REVIEW REQUIRED 且没有阻碍时,停止循环。不要继续寻找新的小问题来为再审一轮找理由。
输出格式
始终按以下结构撰写审查意见。
## PR Review: [PR title, branch, or concise filename summary]
### Merge decision
**[✅ SELF-MERGE OK / 👥 HUMAN REVIEW REQUIRED / 🛑 BLOCKED]**
- **Criticality:** [P0/P1/P2/P3] — [surface], [change class][, +1 modifier: reason]
- **Why:** [the specific gate that fired, with the file or fact that triggered it]
- **Reviewer:** [named CODEOWNER, blame-familiar engineer, or role — omit if self-merge]
- **Focus on:** [1–3 concrete things — omit if self-merge]
- **Observe it:** [command to run + expected output — omit if not observable]
- **Effort:** [~N minutes, which files matter]
- **To self-merge next time:** [the concrete verification or stated intent that would clear the gate — never "this surface always routes to a human"]
[If oversized: add a 🔀 **Consider splitting** line naming the seams.]
### Score
**[✅ for 4/5 or 5/5; ❌ for 1/5–3/5] [N]/5 — [Label]**
[One direct sentence explaining what drove the score. Score is code quality; the merge
decision above is separate.]
---
### Summary
[2–4 sentences. Say what the PR does, what is good, and the main concern.]
---
### Ticket correspondence
[Only when the PR title contains `[CU-<id>]`; omit otherwise.]
**[CU-<id>] [ticket name]** — [link if returned] — **Corresponds / Partial / Divergent / Not verified**
[Quote the ticket's core ask, then name the behavior the diff implements and where —
or what it built instead. If not verified, say why.]
---
### What this could break
[Table of grounded scenarios: what breaks / trigger / who's affected / detection /
rollback. Omit the table for P3 and say instead: "Bounded — [one-sentence boundary]."]
---
### Prior attempts in this code path
[Only when history was checked AND is relevant. State findings with commit SHAs or PR
numbers: earlier fixes, reverts, hotfix chains, what the last attempt missed, and
whether this PR addresses it. If clean, one line: "No prior fix attempts or reverts on
these paths in the last 6 months." If not checked: "Not checked — no repository
access."]
---
### Issues
[If no issues: "No blocking or meaningful issues found."]
**[severity] [category] — path/to/file.ext:line_or_function**
**Confidence:** High/Medium
[Explain: When X happens, Y breaks because Z. Tie it to user/dev impact.]
```language
// ❌ Current
[short relevant snippet]
// ✅ Suggested
[short suggested fix]
```
[按严重 → 主要 → 次要的顺序列出问题。]
小问题
path:line— [可选的一行清理建议]
问题 / 假设
[含糊不清的发现和重要假设。具体指出缺少哪些上下文。 不要用此部分回避自行能够明确判断的问题。]
验证
[说明观察到的内容与推断的内容。例如:“运行了板号规范化测试套件——通过。追踪了提供方重试路径,但未实际执行;没有测试覆盖该路径。”P3 可省略。]
做得好的地方
- [针对代码给出具体且真诚的肯定。如果没有具体内容,则省略此部分。]
升级路线图
达到 4/5
- [必须修复的项目。如果已达到 4 分或更高,写“已达到或超过 4/5”。]
达到 5/5
- [润色、测试或一致性方面的项目。如果已达到 5 分,写“已达到 5/5。无需处理。”]
统计
- 更改的文件:N
- 关键性:[tier]([最高级别文件])
- 差异规模:N 行 / N 个文件
- 行为验证方式:[观察 / 阅读的测试 / 静态检查 / 仅推理]
- 合并决定:[结论]
- 审查覆盖范围:[完整 / 部分;若为部分,请说明原因]
- 历史记录检查:[已完成 / 不可用]
- ClickUp 工单:[CU- — 对应 / 部分对应 / 不一致 / 未验证,或“标题中没有”]
- 评分:N/5
- 问题:共 N 个(严重:N,主要:N,次要:N,细节:N)
- 风险最高的区域:[区域]
- 标记的类别:[列表]
- 未验证:[无法检查的内容——若无则省略]
### Output rules
- Keep issues concise. One strong paragraph plus a fix is enough.
- Do not include giant snippets. Show only the relevant lines.
- If there are many findings, prioritize the top 10 by severity and impact. Put the rest under "Additional notes" only if useful.
- Never say "LGTM" if you did not review enough context. Say "No issues found in the provided diff" instead.
- Be direct: "This can create duplicate charges" is better than "This might possibly be problematic."
- **Lead with the merge decision.** It is the first thing the author reads and the only thing some authors read.
- **Nit budget: 5 maximum.** Nits never affect the score or the gate. If you have more than five, you are linting. Cut to the five that would actually annoy a reader.
- Do not repeat the gate reasoning in the summary. State it once, in the decision block.
- Never state a verdict the evidence does not support in order to sound decisive. "P1, human review required, and here is the one thing I could not verify" is a stronger answer than false certainty in either direction.
- The score icon is deterministic: use ✅ for 4/5 or 5/5 and ❌ for 1/5, 2/5, or 3/5.
---
## Verdict calibration
Read `references/merge-gate.md` when you are unsure about a verdict.
---
## Simplicity quick-reference checklist
Before finalizing, run this mini-check for every non-trivial new abstraction or helper:
1. Is this solving a real current requirement, not “maybe later”?
2. Did I search for an existing repo helper/pattern first?
3. Would stdlib/native/platform/database/framework solve this with fewer edge cases?
4. Is an already-installed dependency meant to do this exact job?
5. Does the custom code duplicate caching, retries, validation, parsing, formatting, state management, or authorization?
6. Can I name a concrete risk if this custom code stays?
7. Can I suggest a smaller replacement that preserves behavior?
If the answer to 6 or 7 is no, do not report it as an issue.
---
## Reviewer self-check before final answer
Before finalizing, ask yourself:
1. Did I only flag PR-introduced or PR-worsened issues?
2. Does every issue have a trigger, impact, cause, and fix?
3. Did I avoid linter/style noise?
4. Did I check contracts across files where relevant?
5. Would a senior engineer accept each finding as useful?
6. Could a cheaper model follow the output without guessing?
### Gate self-check
7. Did I assign the tier from what the changed lines actually govern, opening the hunks rather than reading folder names?
8. Did I take the **highest** hunk's tier, not an average, and apply +1 irreversible only when revert cannot restore already-happened customer state?
9. Did I check prior attempts, or explicitly say I could not — without treating a skip as a gate?
10. Is every break-analysis row grounded in traced code rather than imagined?
11. Did I run the gates in order, and can I quote the exact gate that fired?
12. If I said SELF-MERGE OK: does every Gate 3 condition pass, and was silent/expensive behavior actually observed — including a verified P0?
13. If I said HUMAN REVIEW REQUIRED: did I name a specific reviewer or role, specific focus areas, and the concrete verification that would clear the gate — not "this surface always needs a human"?
14. Am I gating on unverified risk or a real finding, or on folder name / "it's user-facing" / "it's an email"? Vagueness is my problem to resolve, not the author's to absorb.
15. Is the merge decision consistent with the findings and the stated coverage — not merely with the tier?
16. If the PR title had `[CU-<id>]`, did I read the ticket and compare its ask to the diff's behavior — and if the title had no token, did I skip ClickUp entirely?
If not, revise before sending.
A condensed tick-list version of every gate in this skill, for use while reviewing
rather than after: `references/checklist.md`.