code-review

name	code-review
description	以架构师视角审查代码变更，关注模块边界、DRY 复用性、测试完整性、协议合规和长期可维护性。当需要审查 PR、工作区变更或提交代码时使用。
argument-hint	<可选：PR 编号、commit range 或文件路径，留空则审查当前工作区变更>
model	opus

Code Review — 架构级审查

目标不是"代码能不能跑"，而是"这段变更是否让项目更健康"。

复用：本 skill 的审查 rubric 是全项目唯一来源。code-reviewer 子代理（隔离上下文消偏见）与 add-feature / fix-issue 的内嵌审查门控均复用本 rubric，不另写。内嵌门控流水线见 skills/code-review/resources/embedded-review-gate.md。

---

Step 1：确定审查范围

根据输入确定代码变更集：

输入	取变更方式
无参数	git diff + git diff --cached
PR 编号	gh pr diff <number>
commit range	git diff <range>
文件路径	直接审查指定文件

输出变更文件清单，按模块/crate 分组，标注变更类型（新增/修改/删除）。

重要：读变更涉及的完整文件，不要只看 diff。很多问题只有在上下文中才能发现。

---

Step 2：架构合理性审查

2.1 模块边界

检查变更是否违反项目的分层/依赖方向约束。

按项目参见 {baseDir}/resources/<project>.md "模块边界"章节。

2.2 公开 API 变更

如果变更引入了新的公开类型/方法：

• 是否需要在导出文件中增加 re-export？
• 是否遵循现有命名风格？

2.3 短视模式检测（🔴 级别）

逐条检查，一经发现标记 🔴：

• 绕过式修复：不解决根因，只绕过症状（catch-all 吞异常、无条件默认值、类型逃逸）
• 过度特化：为单一场景硬编码，而非复用已有抽象
• 破坏已有抽象：已有基类/trait/builder 可用却不使用，另起炉灶
• 只修一处：同类 bug 在多处出现但只修了报告的那一处

---

Step 3：DRY 与复用性审查

3.1 检查是否重复已有抽象

搜索项目中已有的工具函数、基类、公共模块，确认变更是否与之重复。如已有封装能力不足，正确做法是增强已有封装而非新建。

3.2 检查跨文件代码复制

对新增函数/逻辑块，搜索是否已有相似实现。重点关注：错误处理逻辑、超时/重试逻辑、序列化辅助函数。

各项目的已有抽象和已知重复区域参见 {baseDir}/resources/<project>.md。

---

Step 4：协议合规检查

涉及协议类型/事件/数据结构的变更，此步骤为强制检查。

4.1 A2C 协议合规

变更涉及 SMCP 协议类型时：

• 对照 a2c-smcp-protocol 的 docs/specification/ 规范文档
• 确认字段名、类型、Optional 标记、序列化格式与协议一致
• 确认跨 SDK 兼容性（Python ↔ Rust JSON 序列化结果一致）

4.2 OASP 协议合规

变更涉及 OASP 事件/DTO 时：

• 事件名格式 {namespace}:{action}:{target}
• 字段 camelCase，错误码在正确范围（1xxx-5xxx）
• 双端对齐：office4ai ↔ office-editor4ai 事件名和数据结构一致

协议详情参见 skills/add-feature/resources/a2c.md 和 skills/add-feature/resources/oasp.md。

---

Step 5：测试完整性审查

5.1 测试覆盖

变更类型	测试要求
新增公开 API	必须有单元测试
Bug 修复	必须有复现测试（修复前失败、修复后通过）
行为变更	现有测试是否需要更新
新增事件/DTO	必须有序列化/反序列化测试

5.2 欺骗性测试检测（🔴 级别）

一经发现直接标记 🔴 阻塞合并：

• 无断言测试：只执行代码不验证结果
• 永真断言：assert True / assert!(true) 等与被测逻辑无关
• 吞没错误：对返回值不做检查，或 catch 后静默忽略
• 空 Mock：Mock 返回硬编码成功值，测试永远通过
• 只测 happy path：忽略错误路径和边界条件

判定标准：如果被测函数实现替换为空函数，测试还能通过吗？如果能，就是欺骗性测试。

5.3 测试覆盖度门控（🔴 级别）

变更不得导致测试覆盖率下降。审查时须运行覆盖率检查，确认达到项目要求的最低标准：

各项目覆盖率要求和检查命令参见 {baseDir}/resources/<project>.md "测试覆盖度"章节。

如果变更新增了代码但未新增对应测试，导致覆盖率下降，标记 🔴 阻塞合并。

5.4 测试跳过策略

测试默认不允许跳过。仅在依赖外部重量级服务时允许，且必须注释说明原因。不得跳过因代码缺陷而失败的测试。

测试约定按项目参见 {baseDir}/resources/<project>.md。

---

Step 6：上游依赖问题检查

检查变更中是否存在对上游问题的不当回避：

• 是否有因上游 bug 而 skip 的测试？（不可接受）
• 是否有绕过上游问题的 workaround 但没有 Bug Report？
• 是否有错误被静默吞掉而实际根因在上游？

仅适用于有上游依赖的项目（如 tfrobot-client → rust-sdk）。

---

Step 7：输出审查报告

## 审查摘要

- 审查范围：<变更文件数、涉及模块>
- 总体评价：✅ 可合并 / ⚠️ 需修改后合并 / ❌ 需重新设计

## 发现的问题

### 🔴 必须修复（阻塞合并）
<编号>. <文件:行号> — <问题描述> — <修复建议>

### 🟡 建议改进（不阻塞但推荐）
<编号>. <文件:行号> — <问题描述> — <改进方向>

### 🟢 值得肯定
<变更中做得好的地方——好的抽象、好的测试覆盖、消除技术债务等>

## 测试覆盖评估

- 覆盖率变化：<变更前> → <变更后>（是否达标：✅/❌）
- 新增/修改的公开 API 是否有测试：✅/❌
- 测试是否遵循项目约定：✅/❌
- 建议补充的测试用例：<列表>

只报告发现的问题，没问题的维度不需要列出。

---

Step 8：验证建议

审查完成后，建议变更作者执行项目对应的验证命令。

验证命令按项目参见 {baseDir}/resources/<project>.md。