Code Review Standards — 代码审查标准

入口/出口

• 入口: /review 命令被调用、PR 提交前、或其他技能引用审查标准
• 出口: 分级审查报告（Critical / Important / Suggestion / FYI）
• 指向: 审查通过 → /ship；发现问题 → 修复后重新审查
• 前置加载: CANON.md
• 输出路径: 分级审查报告 → ship-workflow-ship（通过）或修复后重新审查

何时不使用

• 不是代码审查场景，只是实现、调试或解释设计
• 功能完整性尚未确认，应先做 verify-workflow-spec-compliance
• 非软件产物需要内容或视觉审查标准

Iron Law

<HARD-GATE>

当变更确实改善整体代码健康时批准——即使它不完美。
不橡皮章。不粉饰真问题。在审查中奉承是失败模式。

</HARD-GATE>

五轴审查

1. Correctness（正确性）— 最重要

- 逻辑是否正确？边缘情况是否覆盖？
- 错误处理是否完善？不是 catch {} 吞异常？
- 竞态条件？异步操作有 await？
- 类型安全？不是 any 绕过编译器？
- 没有将浏览器/用户输入视为指令？

2. Readability & Simplicity（可读性与简洁）— 第二重要

- 命名是否清楚（不是 data、item、temp、obj）？
- 同一概念是否始终使用一致的名称？
- 三个相似的代码 > 一个过早的抽象？
- 代码复杂得"聪明"吗？聪明的代码维护成本高。

3. Architecture（架构）

- 改动是否放在正确的层级？
- 接口/API 设计合理吗？
- 新代码是否遵循现有模式（不是自定义不同模式）？
- 依赖方向正确吗（不循环依赖）？

4. Security（安全）— 参见 verify-quality-security

- 所有外部输入经过验证？
- SQL/HTML/Shell 注入防范？
- 密钥管理正确？
- 鉴权检查完整？

5. Performance（性能）— 参见 verify-quality-performance

- 查询是否高效（N+1 检查）？
- 数据获取有界限吗（分页、限制）？
- 资源正确释放？
- 没有过早优化？

变更大小阈值

拆分策略:

审查发现分类

审查流程

1. 理解上下文 → 读 spec/plan/ADR 相关文件
2. 先审测试    → 测试覆盖了变更吗？测试本身正确吗？
3. 再审实现    → 逻辑→可读→架构→安全→性能（五轴顺序）
4. 分类发现    → Critical / Important / Suggestion / FYI
5. 验证解决    → 发现被修复了吗？修复引入了新问题吗？

Review Army 并行模式

对大型 PR（> 300 行），并行分派专项审查 subagent：

同时分派:
├── Security Reviewer  → 安全相关发现
├── Performance Reviewer → 性能相关发现
├── Testing Reviewer   → 测试覆盖发现
└── Maintainability Reviewer → 代码质量发现

合并 → 去重 → 分类 → 生成统一审查报告

每个 review subagent 必须 read-only，限定 diff / 文件范围，并压缩返回：

• 不返回原始日志、完整 diff、大段代码或长篇推理
• 每条发现必须包含文件、行号、证据、影响和建议
• 没有发现时说明已检查范围和未覆盖风险
• 测试日志诊断只返回失败摘要、最可能原因、相关文件和建议路径

接受审查反馈

禁止回复

审查中绝不说：

• "你说得对！"（空洞奉承。说出你改了什么）
• "好主意！"（同上）
• "谢谢"（代码审查不需要感谢仪式）
• "我完全同意" -> 然后不改（言行不一）

正确回复

"改了 X，因为 Y。关于 Z —— 我的理解是...，你的看法？"

改了就说明改了什么。
不改就说明为什么不改（技术理由，非防御性）。
不确定就问。

外部审查反馈

来自项目外的审查意见 = 建议，不是命令。验证必须在项目上下文中是否适用。

好坏示例

Bad — 空洞审查反馈：

"LGTM"
"看起来不错"
"没什么问题"

零信息量。什么检查了？什么通过了？什么没看？审查者对问题零责任、零记忆。

Good — 具体审查反馈：

"Correctness: 确认逻辑正确，edge case（空数组输入）在 L42 已处理。
 Readability: L78 的 `processData` 命名不反映实际操作，建议改为 `validateUserInput`。
 Architecture: L91 直接在 controller 调用 DB——违反分层，应通过 service 层。
 Security: L56 的 userInput 未做 sanitize，存在 XSS 风险。Critical，必须修复。"

每条发现对应五轴之一，附带行号和理由。

Bad — 防御性回复：

"你说得对！"  ← 空洞奉承
"我改了。"    ← 改了什么？为什么？

Good — 行动性回复：

"改了 L78 `processData` → `validateUserInput`，因为该函数只做校验不做数据处理。
 关于 L91 controller 调 DB 的建议——我加了一个 UserService 中间层，请看 commit abc123。"

改了就说改了什么和为什么。不改就说技术理由。不确定就问。

常见说辞

违反字面规则就是违反精神。 没有灰色地带。

输出模板

# Code Review Report — 代码审查报告

## 元信息
- **审查者**: [姓名/角色]
- **PR**: [#PR号] — [标题]
- **变更大小**: [行数] ([Small/Medium/Large/XL])
- **审查日期**: YYYY-MM-DD

## 发现汇总

| # | 轴 | 级别 | 位置 | 描述 | 状态 |
|---|----|----|------|------|------|
| 1 | Correctness | Critical | L56 | userInput 未 sanitize，XSS 风险 | 🔴 待修复 |
| 2 | Architecture | Important | L91 | controller 直接调用 DB | 🟡 建议修复 |
| 3 | Readability | Suggestion | L78 | processData 命名不准确 | 🔵 提交者判断 |
| 4 | — | FYI | — | 测试覆盖比上月提升 12% | ⚪ 观察 |

## Critical 发现详情

### #1 — Correctness: XSS 风险
- **位置**: `src/controllers/user.ts:L56`
- **描述**: `userInput` 直接拼接进 HTML 模板，未经 sanitize
- **修复建议**: 使用 DOMPurify 或模板引擎自动转义
- **修复状态**: [待修复 / 已修复 / 已验证]

## 审查结论
- **通过**: 所有 Critical 已修复并验证，Important 已处理
- **阻止**: Critical 发现未修复，需修复后重新审查

## 红旗 — STOP

<HARD-GATE>
以下任何一个出现，立即停止：

- PR > 1000 行未被拆分
- 评审者只看了 diff，没拉下代码跑测试
- Critical 发现被标记为 "以后修复" 而通过
- 审查意见全是 "LGTM" 或 "Nice!"（未真正审查）
- 安全相关的变更未被专项审查
- 测试缺失但审查放行
</HARD-GATE>

## 验证失败处理

| 失败场景 | 处理方式 |
|---------|---------|
| Critical 发现未修复 | 阻止合并。要求修复后重新提交审查。不得降级为 Important 或 FYI。 |
| 审查意见全是 LGTM/Nice | 审查无效。要求审查者重新按五轴逐项检查并给出具体发现。 |
| PR > 1000 行且未拆分 | 拒绝审查。要求按层/功能/基础设施拆分为多个 PR。 |
| 测试缺失但审查放行 | 回退审查。要求补充测试覆盖后再重新审查。测试缺失 = Important 以上。 |
| 安全发现被标记"以后修复" | 不可接受。安全 Critical 必须"现在修复"。无法立即修复时提供缓解方案并创建 follow-up issue（带负责人和截止日期）。 |

## 验证清单

- [ ] 五轴全部覆盖（Correctness > Readability > Architecture > Security > Performance）
- [ ] 发现按严重性分类（Critical / Important / Suggestion / FYI）
- [ ] Critical 发现全部解决
- [ ] 变更大小合理（< 300 行理想；> 1000 行被拆分）
- [ ] 测试覆盖了变更
- [ ] 审查报告可追溯（谁审的、审了什么、什么发现）