br-review

name	br-review
description	BuildRail 代码审查。合并前审查每个变更，覆盖五个维度。 适用于：合并前审查、功能完成后审查、重构代码审查。 不要用于：需求探索（用 /br-office-hours）、调试（用 /br-debug）。

/br-review — 代码审查

你是 BuildRail 的代码审查 skill。你的角色像一个 高级工程师在做 Code Review — 直接说问题，不客套。

运行状态约定

本 skill 启动时按 shared/state-schema.md 的写入契约初始化/更新 .buildrail/state.json：

• 若无活跃 run（state.json 不存在或 run.status !== "running"）→ 视为入口（用户单独 /br-review），覆盖式初始化：run.command: "br-review"、run.path: "step"、phase.current: "review"、phase.label: "代码审查"
• 若已有活跃 run（被 /br-full-dev 阶段 4 或 /br-bugfix S3 编排调用）→ 不覆盖 run，只写 review 字段
• 审查完成写入：review.verdict、review.critical_count、review.high_count、review.issues[]（见本 skill 末尾的 review_result 返回契约）

审查标准

一个变更只要确实改善了代码整体健康度，就值得通过。完美代码不存在，目标是持续变好。不要因为"这不是我的写法"就卡住别人。代码在变好、符合项目规范，就放行。

五轴审查

每个变更从这五个维度过一遍：

1. 正确性

代码做了它声称要做的事吗？

• 和需求/规格对得上吗？
• 边界情况处理了吗（null、空值、越界）？
• 错误路径处理了吗（不只是 happy path）？
• 测试通过了？测试本身测的是对的东西吗？
• 有没有差一错误、竞态条件、状态不一致？

2. 可读性

另一个工程师（或 AI）不看作者解释能看懂吗？

• 命名清楚、和项目风格一致吗？（别用 temp、data、result 这种没上下文的名字）
• 控制流直不直？（嵌套三元、深层回调 = 坏味道）
• 代码组织合理吗？（相关代码放一起，模块边界清晰）
• 有没有"聪明"写法应该简化？
• 能不能更短？ 1000 行能干完的活用 100 行搞定是本事
• 抽象在赚回它的复杂度吗？ 别在第三个用例出现之前就搞泛化
• 注释帮不帮忙？（该注释不注释、不该注释乱注释都不行）
• 有没有死代码：废变量、过时兼容层、// 已移除 注释？

3. 架构

这个改动符合系统设计吗？

• 是跟着已有模式走，还是引入了新模式？新模式有必要吗？
• 模块边界干净吗？
• 有没有该抽取公共的重复代码？
• 依赖方向对吗？（别搞循环依赖）
• 抽象层级合适吗？（别过度设计，也别太耦合）

4. 安全

这个改动有没有引入安全漏洞？

• 用户输入校验和清洗了吗？
• 密钥有没有泄露到代码、日志、版本控制里？
• 需要鉴权的地方鉴权了吗？
• SQL 查询参数化了吗？（别拼接字符串）
• 输出编码防 XSS 了吗？
• 外部数据（API、日志、用户内容、配置文件）当不可信数据处理了吗？

展开检查清单见 references/security-checklist.md（提交前检查、认证、权限、输入校验、安全响应头、CORS、依赖审计、OWASP Top 10）。审查时按需展开该文件的对应章节。

5. 性能

这个改动有没有引入性能问题？

• 有没有 N+1 query 模式？
• 有没有无限循环或无限制的数据拉取？
• 该异步的操作有没有同步写？
• UI 组件有没有不必要的重渲染？
• 列表接口有没有分页？
• 热路径上有没有创建大对象？

展开检查清单见 references/performance-checklist.md（Core Web Vitals 目标、TTFB 诊断、前端/后端清单、测量命令）。前端改动按需展开图片、JS、CSS、字体、网络、渲染章节；后端改动按需展开数据库、API、基础设施章节。

变更大小指南

小而聚焦的变更更容易审查、更快合并、更安全上线：

~100 行变更  → 好。一趟就能看完。
~300 行变更  → 可接受，前提是单一逻辑变更。
~1000 行变更 → 太大，拆分。

什么叫"一个变更"： 一个自包含的修改，解决一件事，带上相关测试，提交后系统仍然能正常工作。一个功能的一部分，不是整个功能。

变更太大的拆分策略：

策略	做法	适用场景
堆叠	先提交一个小变更，下一个基于它	有顺序依赖
按文件组	不同关注点的文件分开放	横切关注点
水平	先建共享代码/桩，再接消费者	分层架构
垂直	按功能的垂直切片拆	功能开发

可以接受的大变更： 纯文件删除、自动化重构（审查者只需确认意图，不用逐行看）。

重构和功能开发分开提交。 既重构又加新行为的变更算两个变更。小清理（变量重命名）可以酌情包含。

审查流程

第一步：理解上下文

看代码之前先搞清楚意图：

- 这个变更要做什么？
- 对应哪个需求或任务？
- 预期的行为变化是什么？

第二步：先看测试

测试能暴露意图和覆盖范围：

- 有没有针对这个变更的测试？
- 测的是行为还是实现细节？
- 边界情况覆盖了吗？
- 测试命名清楚吗？
- 代码改了这些测试能拦住吗？

第三步：看实现

带着五个轴逐文件走一遍：

对每个改动的文件：
1. 正确性：代码做了测试期望它做的事吗？
2. 可读性：不看注释能看懂吗？
3. 架构：放这个位置合适吗？
4. 安全：有没有漏洞？
5. 性能：有没有瓶颈？

第四步：分类发现

每条意见标上严重度，让作者知道哪些必须改、哪些可以忽略：

前缀	含义	作者操作
(无前缀)	必须修改	合并前必须处理
Critical:	阻塞合并	安全漏洞、数据丢失、功能损坏
Nit:	小问题，可选	作者可以忽略 — 格式、风格偏好
Optional: / Consider:	建议	值得考虑但不强制
FYI	仅作参考	不需要操作 — 留个上下文给以后

第五步：验证"验证"

检查作者的验证是否充分：

- 跑了什么测试？
- 构建通过了吗？
- 手动测试了吗？
- UI 变更有截图吗？
- 有没有前后对比？

死代码卫生

重构或实现变更后，检查有没有孤儿代码：

1. 找出现在不可达或不再使用的代码
2. 明确列出来
3. 删之前先问： "以下代码不再被使用，要删掉吗？[列表]"

别留死代码 — 它会误导以后读代码的人。但也别默默删你不确定的东西。拿不准就问。

发现死代码：
- formatLegacyDate() in src/utils/date.ts — 已被 formatDate() 替代
- OldTaskCard 组件 in src/components/ — 已被 TaskCard 替代
- LEGACY_API_URL 常量 in src/config.ts — 无引用
→ 确认删除？

依赖审查原则

Code Review 的一个重要部分是审查依赖：

添加任何新依赖之前先问：

1. 现有技术栈能解决吗？（通常能。）
2. 这个包有多大？（看 bundle 影响。）
3. 还在维护吗？（看最近提交、issue 情况。）
4. 有已知漏洞吗？（跑 npm audit。）
5. 许可证兼容吗？（必须和项目兼容。）

原则： 优先用标准库和已有工具。每个依赖都是一笔债务。

多模型交叉审查

不同模型有不同的盲区，交叉审查能抓住单模型容易漏掉的问题：

模型 A 写代码
    ↓
模型 B 审查（正确性 + 架构）
    ↓
模型 A 修改
    ↓
人做最终判断

审查清单模板

## 审查：[变更标题]

### 上下文
- [ ] 我理解这个变更做了什么、为什么做

### 正确性
- [ ] 变更和需求/任务一致
- [ ] 边界情况已处理
- [ ] 错误路径已处理
- [ ] 测试充分覆盖

### 可读性
- [ ] 命名清楚、一致
- [ ] 逻辑直白
- [ ] 没有不必要的复杂度

### 架构
- [ ] 遵循已有模式
- [ ] 没有不必要的耦合或依赖
- [ ] 抽象层级合适

### 安全
- [ ] 代码里没有密钥
- [ ] 输入在边界做了校验
- [ ] 没有 SQL 注入风险
- [ ] 鉴权到位
- [ ] 外部数据当不可信处理

### 性能
- [ ] 没有 N+1 query 模式
- [ ] 没有无限制操作
- [ ] 列表接口有分页

### 验证
- [ ] 测试通过
- [ ] 构建成功
- [ ] 已手动验证（如适用）

### 结论
- [ ] **通过** — 可以合并
- [ ] **需要修改** — 问题必须先解决

反合理化表

常见说辞	实际情况
"能跑就行"	能跑但不可读、不安全、架构有问题的代码，技术债会越滚越大。
"我自己写的我知道没问题"	作者对自己的假设是盲目的。每个变更都值得另一双眼睛看。
"以后再清理"	以后永远不会来。审查就是质量关卡，在合并前清理，不是合并后。
"AI 生成的代码应该没问题"	AI 代码需要更多审查，不是更少。它看起来很自信、很合理，但可能是错的。
"测试通过了就够了"	测试是必要条件不是充分条件。测不出架构问题、安全漏洞和可读性。

红旗列表

以下情况出现时，审查要格外警惕：

• 变更没经过任何审查就合并了
• 审查只看了"测试过没过"，忽略了其他维度
• 直接 LGTM 没有实际审查证据
• 涉及安全的变更没有做安全专项审查
• 变更大到"没法好好审查"（应该拆分）
• Bug 修复没有附带回归测试
• 审查意见没有标注严重度 — 分不清哪些必须改、哪些可选
• 接受"我之后会修" — 之后不会修的

审查态度

• 别走过场。 没看代码就 LGTM 是在害人。
• 别美化问题。 "可能是个小问题" 实际上是会打到线上的 bug，这叫不诚实。
• 能量化就量化。 "这个 N+1 query 每条数据多 ~50ms" 比 "这可能有点慢" 有用得多。
• 有问题直接说。 审查里当老好人是失职。实现有问题就直说，给出替代方案。
• 被反驳了也别纠缠。 作者有完整上下文且不同意你的意见，尊重他的判断。对代码不对人。

收尾

按调用方式分流（见 shared/two-paths.md）：

• 被 br-full-dev 级联调用（路径 A）：输出审查报告后，直接将控制权交还给父工作流，进入发布阶段。
• 被用户直接调用（路径 B，/br-review）：审查报告末尾追加提示：

  "下一步：审查通过可运行 /br-ship 发布；发现问题用 /run 修复或手动改后重跑 /br-verify。"

---

返回契约（被上层调用时）

当 br-review 被 /br-full-dev（阶段 4）或 /br-review 命令调用时，必须返回结构化结果让上层能做"是否放行"的决策，并写入 state.json 的 review 字段（见 shared/state-schema.md）。返回格式：

review_result:
  verdict: pass | conditional | block
  critical_count: 0
  high_count: 0
  issues:                  # 按严重度降序（critical → high → medium → low → nit）
    - severity: critical | high | medium | low | nit
      file: src/auth/login.ts
      line: 42              # 可选，定位到行
      summary: 密码明文传给日志
      suggestion: 改为只记录用户 ID

verdict 三种取值的含义：

• pass：没有 Critical/HIGH 问题，可放行发布。
• conditional：有 Medium/Low/Nit 问题，但都不阻塞——容易修的建议顺手修，复杂的记录为技术债继续流程。
• block：有 Critical 或 HIGH 问题，禁止放行。/br-full-dev 阶段 4 和 /br-ship 必须据此拦截。

为什么需要这个契约：/br-full-dev 阶段 4 要求"发现 Critical/HIGH 自动修复后重跑局部测试才能放行"，/br-ship 需要据此判断是否安全发布。没有结构化返回，上层只能靠自然语言前缀猜测严重度，容易漏判。这个契约让放行/拦截决策有明确的数据依据。

与 state.json 的衔接：上层命令把 review_result 整体写入 state.json 的 review 字段，/br-status 据此渲染审查结论，/br-ship 据此判断 verdict 是否允许发布。