gstack /review 预合并审查清单全解:两轮分级检查、Fix-First 自动修复与专家子代理体系

review/checklist.md 是 gstack 的 /review 技能在执行"预合并 PR 审查"时的核心规则集。本文以该清单为骨架,系统讲解它的两轮(两 pass)分级审查模型、五大 CRITICAL 类别、十大 INFORMATIONAL 类别、"Fix-First 先修再问"决策启发式以及"DO NOT flag"抑制规则,并结合 review/SKILL.md 的调用流程、review/specialists/ 专家子代理与 test/skill-e2e-review.test.ts 的测试证据,说明这套清单如何在真实审查中被加载、执行并产出可验证结果。读完你将掌握:如何按清单组织一次人工/Agent 预合并审查,如何区分"自动修复"与"必须问人"的问题,以及如何为 Agent 化的 Code Review 设计一套低误报、高覆盖的分级检查体系。

清单的定位:审查流程中"只此一份"的规则源

这份清单不是独立的 lint 规则,而是 gstack 预合并审查的单一事实来源。review/SKILL.md 的 Step 2 明确写道:

Read ~/.claude/skills/gstack/review/checklist.md

并强调"若无法读取该文件,停止并报告错误,不得在没有清单的情况下继续"。同样,ship/sections/review-army.md 和 land-and-deploy/SKILL.md 在各自的审查阶段都会读取同一份文件。这意味着 /review 与 /ship 共享同一条"什么该标、什么不该标、什么该自动修"的边界。CHANGELOG.md 也记录了这一设计:"分类规则集中在一处(review/checklist.md),让 /review 和 /ship 保持同步。"

清单的开头给出了审查对象与总原则:

Review the git diff origin/main output for the issues listed below. Be specific — cite file:line and suggest fixes. Skip anything that's fine. Only flag real problems.

即:审查的是当前分支相对 base 分支的 diff;每个问题必须落到具体 file:line 并给出修复建议;没问题就跳过,只报真问题。

两 pass 模型:先 CRITICAL,后 INFORMATIONAL

清单把审查拆成两个 pass,顺序与严重度绑定:

• Pass 1(CRITICAL):先跑 SQL & Data Safety、Race Conditions、LLM Output Trust Boundary、Shell Injection、Enum Completeness。严重度最高。
• Pass 2(INFORMATIONAL):跑其余类别。严重度较低但仍要处理。
• Specialist 类别(交给并行子代理,不在这份清单里):Test Gaps、Dead Code、Magic Numbers、Conditional Side Effects、Performance & Bundle Impact、Crypto & Entropy,见 review/specialists/。

所有发现都通过 "Fix-First Review" 落地:明显的机械修复直接应用,真正有歧义的问题合并成一个用户提问。

清单给出的标准输出格式是:

Pre-Landing Review: N issues (X critical, Y informational)

**AUTO-FIXED:**
- [file:line] Problem → fix applied

**NEEDS INPUT:**
- [file:line] Problem description
  Recommended fix: suggested fix

若没有发现问题,只输出一行 Pre-Landing Review: No issues found.。并明确要求"Be terse. 每个问题一行描述 + 一行修复,不要前言、不要总结、不要'整体看起来不错'。"

这种"只报真问题 + 强制 file:line + 强制给出修复"的约束,正是清单能同时服务人类读者和 Agent 的关键:输出是结构化、可解析、可回归验证的。test/skill-e2e-review.test.ts 会直接把 review/checklist.md 拷进测试沙箱(变量名 review-checklist.md),再让模型"读取并应用它",用真实模型输出回归验证清单是否真的被遵守。

Pass 1 — CRITICAL:五类必查项

SQL & Data Safety

清单列出的 SQL 与数据安全点:

• SQL 里的字符串插值(即使值被 .to_i/.to_f 强转)——应使用参数化查询(Rails:sanitize_sql_array/Arel;Node:prepared statements;Python:parameterized queries)。
• TOCTOU 竞态:本应原子化的"先查后写"(check-then-set),应改为带 WHERE 的原子 update_all。
• 绕过模型校验直接写库(Rails:update_column;Django:QuerySet.update();Prisma:raw query)。
• N+1 查询:在循环/视图里用到关联却没做预加载(Rails:.includes();SQLAlchemy:joinedload();Prisma:include)。

Race Conditions & Concurrency

• "读-检查-写"缺少唯一约束,或没有捕获重复键并重试(如 where(hash:).first 后 save!,并发插入会失败)。
• find-or-create 没有唯一数据库索引——并发调用会产生重复行。
• 状态流转没走原子化的 WHERE old_status = ? UPDATE SET new_status——并发更新可能跳变或重复应用。
• 对用户可控数据做不安全 HTML 渲染(Rails:.html_safe/raw();React:dangerouslySetInnerHTML;Vue:v-html;Django:|safe/mark_safe)——XSS。

LLM Output Trust Boundary

这是 gstack 针对"Agent 生成内容"专门加的一类,是它区别于传统 lint 清单的核心:

• LLM 生成的值(邮箱、URL、名字)未经格式校验就写库或传给 mailer——应在持久化前加轻量 guard(EMAIL_REGEXP、URI.parse、.strip)。
• 结构化工具输出(数组、hash)在写库前没做类型/形状检查。
• LLM 生成的 URL 未做 allowlist 就抓取——若 URL 指向内网则构成 SSRF 风险(Python:urllib.parse.urlparse → 抓取前用 hostname 对照 blocklist,再 requests.get/httpx.get)。
• LLM 输出未做净化就写入知识库或向量库——stored prompt injection 风险。

Shell Injection (Python-specific)

• subprocess.run() / subprocess.call() / subprocess.Popen() 同时使用 shell=True 且命令串里有 f-string/.format() 插值——应改用参数数组。
• os.system() 带变量插值——替换为使用参数数组的 subprocess.run()。
• 对 LLM 生成的代码做 eval() / exec() 且无沙箱。

Enum & Value Completeness

当 diff 引入了新的枚举值、状态字符串、tier 名或类型常量时,清单要求追踪它流经每一个消费者:

• 读(不只是 grep——要 READ)每个对该值做 switch/过滤/展示的文件。若任一消费者没处理新值,标出。常见漏点:前端下拉框加了新值,但后端模型/计算逻辑没持久化它。
• 检查 allowlist/过滤数组。搜索包含同级值的数组或 %w[] 列表(例如往 tiers 加 "revise",就找到所有 %w[quick lfg mega],确认 "revise" 在该在的地方都加了)。
• 检查 case/if-elsif 链:若现有代码对枚举分支,新值是否会落到错误的默认分支。

清单特别强调这一步"需要在 diff 之外读代码":用 Grep 找到所有引用同级值的位置(如 grep "lfg" 或 "mega" 找所有 tier 消费者),逐个读。在 review/SKILL.md 的 Step 4 中,这也正是"唯一一个 diff 内审查不够、必须外扩读代码"的类别。test/skill-e2e-review.test.ts 专门构造了一个场景,让模型"特别留意 Enum & Value Completeness 一节",回归验证这一能力。

Pass 2 — INFORMATIONAL:十类信息级检查

清单把"没那么致命但值得修"的问题集中到 Pass 2,便于与 CRITICAL 区分呈现:

• Async/Sync Mixing(Python-specific):async def 端点里出现同步的 subprocess.run()、open()、requests.get()——会阻塞事件循环,应用 asyncio.to_thread()、aiofiles、httpx.AsyncClient;async 里 time.sleep() 应用 asyncio.sleep();异步上下文里的同步 DB 调用没有 run_in_executor() 包裹。
• Column/Field Name Safety:核对 ORM 查询(.select()、.eq()、.gte()、.order())里的列名与真实 DB schema 是否一致——错误列名会静默返回空结果或抛出被吞掉的错误;检查查询结果的 .get() 用的是否是被 select 的列名;有 schema 文档时交叉核对。
• Dead Code & Consistency(仅版本/changelog):PR 标题与 VERSION/CHANGELOG 文件间版本不一致;CHANGELOG 条目描述不准确(例如写"从 X 改成 Y"但 X 从未存在)。清单注明:其余可维护性问题交给 maintainability 专家。
• LLM Prompt Issues:prompt 里的 0 起始列表(LLM 稳定地返回 1 起始);prompt 文本列出的可用工具/能力与 tool_classes/tools 数组里实际接线的不一致;同一个词/token 上限在多处声明、可能漂移。
• Completeness Gaps:凡"完整版成本 <30 分钟 CC 时间"的偷懒实现(部分枚举处理、不完整的错误路径、明显能补的边缘情况);只给人类团队工时估算的选项——应同时给出人类与 CC+gstack 时间;加缺失测试只是"湖"不是"海"时的测试覆盖缺口;80–90% 实现但用少量额外代码就能到 100% 的功能。
• Time Window Safety:按日期键查询假设"今天"覆盖 24 小时——PT 早上 8 点的报告用今天的键只能看到午夜到早上 8 点;相关功能时间窗口不匹配(一个用小时桶,另一个对同一数据用日键)。
• Type Coercion at Boundaries:值在 Ruby→JSON→JS 边界类型可能变化(数字 vs 字符串)——hash/digest 的输入必须做类型归一;hash/digest 输入没在序列化前调 .to_s 或等价物——{ cores: 8 } 与 { cores: "8" } 会算出不同 hash。
• View/Frontend:partial 里内联 <style> 块(每次渲染都重解析);视图里 O(n*m) 查找(循环里 Array#find 而不是 index_by hash);在 DB 结果上做 Ruby 侧 .select{} 过滤,本可下沉为 WHERE 子句(除非是有意避免前导通配 LIKE)。
• Distribution & CI/CD Pipeline:CI/CD 工作流变更(.github/workflows/):验证构建工具版本与项目要求一致、artifact 名/路径正确、secrets 用 ${{ secrets.X }} 而非硬编码;新增 artifact 类型(CLI 二进制、库、包):确认存在发布/发布工作流且目标平台正确;跨平台构建:CI 矩阵覆盖所有目标 OS/架构,或文档说明哪些未测;版本 tag 格式一致:v1.2.3 vs 1.2.3——必须与 VERSION 文件、git tags、发布脚本一致;发布步骤幂等:重跑发布工作流不应失败(如 gh release create 前先 gh release delete)。

清单对这一类还给出明确的 DO NOT flag 边界,防止误报:

• 已有自动部署管道的 Web 服务(Docker build + K8s deploy)。
• 不对外分发的内部工具。
• 仅测试相关的 CI 变更(加测试步骤,非发布步骤)。

严重度分类:CRITICAL / INFORMATIONAL / SPECIALIST 三分

清单用一个三分图固定了每个类别归属:

CRITICAL (highest severity):      INFORMATIONAL (main agent):      SPECIALIST (parallel subagents):
├─ SQL & Data Safety              ├─ Async/Sync Mixing             ├─ Testing specialist
├─ Race Conditions & Concurrency  ├─ Column/Field Name Safety      ├─ Maintainability specialist
├─ LLM Output Trust Boundary      ├─ Dead Code (version only)      ├─ Security specialist
├─ Shell Injection                ├─ LLM Prompt Issues             ├─ Performance specialist
└─ Enum & Value Completeness      ├─ Completeness Gaps             ├─ Data Migration specialist
                                   ├─ Time Window Safety            ├─ API Contract specialist
                                   ├─ Type Coercion at Boundaries   └─ Red Team (conditional)
                                   ├─ View/Frontend
                                   └─ Distribution & CI/CD Pipeline

All findings are actioned via Fix-First Review. Severity determines
presentation order and classification of AUTO-FIX vs ASK — critical
findings lean toward ASK (they're riskier), informational findings
lean toward AUTO-FIX (they're more mechanical).

这张图的要点是:严重度决定的是"呈现顺序"和"自动修 vs 问人"的倾向,而不是"要不要处理"——所有发现都进入 Fix-First 流程。CRITICAL 倾向 ASK(风险更高),INFORMATIONAL 倾向 AUTO-FIX(更机械)。这与 review/SKILL.md Step 4.5 中"按严重度并行派发专家子代理"的实现一致:review/specialists/ 下每个专家(checklist 头部注释里说的 "handled by parallel subagents")都是独立 .md,由 SKILL.md 在满足 scope 条件时通过 Agent 工具并行启动,每个专家输出统一的 JSON finding 行。

Fix-First 启发式:什么该自动修,什么必须问

清单的 "Fix-First Heuristic" 一节是 /review 与 /ship 共享的"修/问"分界线:

AUTO-FIX (agent fixes without asking):     ASK (needs human judgment):
├─ Dead code / unused variables            ├─ Security (auth, XSS, injection)
├─ N+1 queries (missing eager loading)     ├─ Race conditions
├─ Stale comments contradicting code       ├─ Design decisions
├─ Magic numbers → named constants         ├─ Large fixes (>20 lines)
├─ Missing LLM output validation           ├─ Enum completeness
├─ Version/path mismatches                 ├─ Removing functionality
├─ Variables assigned but never read       └─ Anything changing user-visible
└─ Inline styles, O(n*m) view lookups        behavior

并给出"经验法则":若修复是机械的、资深工程师会不加讨论直接做,就是 AUTO-FIX;若不同工程师可能对修复方案有分歧,就是 ASK。 CRITICAL 发现默认偏向 ASK,INFORMATIONAL 发现默认偏向 AUTO-FIX。

在 review/SKILL.md 的 Step 5 中,这套启发式被具体执行:Step 5a 对每个 finding 按清单分类为 AUTO-FIX 或 ASK;Step 5b 自动应用所有 AUTO-FIX 项,每项输出一行 [AUTO-FIXED] [file:line] Problem → what you did;Step 5c 把剩余 ASK 项合并成一次 AskUserQuestion,每项给 A) Fix / B) Skip 选项并附总体建议;Step 5d 应用用户批准的修复。清单里还有一个专门的"test stub 覆盖"规则:任何带 test_stub 字段(由专家生成)的发现,无论原分类如何,一律重分类为 ASK——因为创建测试文件属于"用户可见行为变更",需用户批准。

Suppressions:清单明确"不要标"的反误报规则

清单末尾的 "Suppressions — DO NOT flag these" 是一份低误报的关键约束,值得单独列出:

• 无害且有助可读性的冗余(如 present? 与 length > 20 冗余)不标"X 与 Y 冗余"。
• 不标"加注释解释这个阈值/常量为什么这么选"——阈值在调优期会变,注释会腐化。
• 当断言已覆盖该行为时,不标"这个断言可以更紧"。
• 不建议纯为一致性做的改动(把某个值包进条件,以对齐另一个常量的守卫方式)。
• 输入受约束、X 实际不会发生时,不标"正则没处理 X 这个边界"。
• 不标"测试同时覆盖了多个 guard"——测试本就不必隔离每个 guard。
• 不标 eval 阈值变更(max_actionable、min scores)——这些是经验调出来的、常变的。
• 无害的空操作(如对一个数组里永远不会出现的元素做 .reject)。
• 任何当前正在审查的 diff 已经处理过的问题——评论前先读完整 diff。

配合 review/SKILL.md 的 "Important Rules"("读完整 diff 再评论;只标真问题;Be terse")与 "Pre-emit verification gate"(每个 finding 必须引用触发它的具体代码行,引用不出来就把置信度强制压到 4–5 并压进附录),这份 Suppressions 规则构成了"低误报 + 可验证"的双层防线。

清单如何被加载与验证:源码与测试证据

从调用链看,清单不是静态文档:

1. 加载:review/SKILL.md Step 2 要求读取清单,读不到就 STOP。/ship 与 /land-and-deploy 的审查阶段(见 ship/sections/review-army.md、land-and-deploy/SKILL.md)也读取同一份文件,保证两处审查口径一致。
2. 应用:review/SKILL.md Step 4(Critical pass)把清单的 CRITICAL 类别应用到 diff;Step 4.5/4.6 把专家类别(对应清单头部注释里的 specialist 项)交给并行子代理,按 JSON finding 行合并去重,并计算 PR Quality Score。
3. 回归验证:test/skill-e2e-review.test.ts 在多个用例里直接把 review/checklist.md 拷进沙箱,指示模型"读取并应用 review-checklist.md",并在 Enum & Value Completeness 用例里特别提示"重点看 Enum & Value Completeness 一节"。这证明清单的每一节都被当成可被模型执行、可被回归测试断言的行为规范,而非纯说明文档。

清单与专家文件的分工也是清晰可验证的:review/specialists/ 下每个专家文件(如 review/specialists/security.md、review/specialists/testing.md)顶部都声明了自己的 Scope(例如 security 专家是 SCOPE_AUTH=true OR (SCOPE_BACKEND=true AND diff > 100 lines))和统一的 JSON finding schema(severity/confidence/path/line/category/summary/fix/fingerprint/specialist)。这正是清单头部"Specialist categories handled by parallel subagents, NOT this checklist"的实现落点——主清单管主代理的 CRITICAL/INFORMATIONAL,专家文件管子代理的深度专项,两者通过同一套 fingerprint 去重机制在 Step 4.6 合并。

实操要点:如何把这份清单用在一次预合并审查

综合清单与 review/SKILL.md,一次标准审查的执行顺序是:

1. 确认 base 分支与 diff 存在:git fetch origin <base> 后计算 DIFF_BASE=$(git merge-base origin/<base> HEAD),git diff "$DIFF_BASE" 得到完整 diff(含未提交改动)。
2. 先做 scope 检查(Step 1.5):对照 TODOS.md/commit 信息判断是否 SCOPE CREEP 或 REQUIREMENTS MISSING,输出一行 Scope Check: CLEAN / DRIFT / MISSING。
3. 按清单跑两 pass:Pass 1 的 CRITICAL 五类优先(其中 Enum & Value Completeness 必须外扩读代码),再跑 Pass 2 的 INFORMATIONAL。
4. 触发专家子代理(若满足 scope 且 DIFF_LINES 达阈值),收集 JSON findings,按 fingerprint 去重、按置信度分级。
5. Fix-First 落地:AUTO-FIX 直接改并逐行汇报;ASK 合并成一个提问,附推荐;带 test_stub 的一律 ASK。
6. 输出:严格按清单的 Pre-Landing Review: N issues (X critical, Y informational) 格式,AUTO-FIXED 与 NEEDS INPUT 分列;没有问题则输出一行 Pre-Landing Review: No issues found.。

这套清单的价值在于:它把"资深工程师预合并前会扫什么"固化成了一份可被 Agent 精确执行、可被 e2e 测试回归、且能同时服务 /review 与 /ship 的单一规则源。对想为自己项目设计 Agent 化 Code Review 的读者,清单给出的方法论同样可直接借鉴:两轮分级(先致命后次要)、明确的"该标/不该标"正反清单、机械修复与人审的清晰分界,以及强制 file:line + 修复建议 + terse 输出的结构化约束。