code-review

# Skill: code-review

## 描述

L2 特性级多维度代码审查。Feature 全部任务完成后执行。

## 触发

```
/code-review --base 2.5.0-PM
```

---

## 审查流程

1. 执行 `git diff 2.5.0-PM...HEAD --stat` 获取变更文件列表
2. 执行 `git diff 2.5.0-PM...HEAD` 获取完整 diff
3. 对照 Feature 的 `spec.md`、`design.md` 和 `tasks.md`
4. 按 7 维度逐一审查
5. 输出审查报告

---

## 7 维度审查框架

### 维度 1：边界条件

| 检查项 | 说明 |
|--------|------|
| null/None 处理 | 外部输入是否校验 None/空字符串 |
| 空集合 | 列表/字典为空时是否正确处理（不抛异常） |
| 数值边界 | 分页 page/size 合法性、ID 为 0/-1 |
| 字符串长度 | 数据库字段长度限制是否在 API 层校验 |
| 超时处理 | 外部调用（LLM/MCP/HTTP）是否设置超时 |
| 分页溢出 | 请求超出总页数时返回空列表而非错误 |

### 维度 2：权限与认证

| 检查项 | 说明 |
|--------|------|
| 认证注入 | 需要认证的端点是否使用 `UserPayload = Depends(UserPayload.get_login_user)` |
| 五级权限链路 | 是否遵循：super_admin → tenant 归属 → tenant admin → ReBAC → RBAC 菜单 |
| PermissionService | 权限检查是否走 `PermissionService.check()` 而非直接查旧表 |
| 资源授权 | 创建资源时是否调用 `PermissionService.authorize()` 写入 owner 元组 |
| tenant_id 隔离 | 跨租户访问是否被阻止（SQLAlchemy event 自动注入） |
| WebSocket 认证 | WS 端点是否使用 `UserPayload.get_login_user_from_ws` |

### 维度 3：并发安全

| 检查项 | 说明 |
|--------|------|
| OpenFGA 双写 | MySQL + OpenFGA 写入是否有失败补偿（failed_tuples 表） |
| 数据库事务 | 多表写入是否在同一事务内 |
| Celery 幂等 | 异步任务是否支持重试不产生副作用 |
| 竞态条件 | 并发创建同名资源是否有唯一约束或乐观锁 |
| 会话状态 | Redis 缓存读写是否考虑过期和并发更新 |

### 维度 4：信息泄漏

| 检查项 | 说明 |
|--------|------|
| 硬编码敏感信息 | 代码中无明文密码/密钥/token |
| 错误信息 | 异常响应不暴露堆栈/SQL/内部路径 |
| 日志脱敏 | logger 输出中敏感字段已脱敏 |
| 前端暴露 | 前端代码不包含后端 IP/密钥/内部 API 路径 |
| tenant_id 泄漏 | API 响应不向前端返回其他租户的 tenant_id |

### 维度 5：测试覆盖

| 检查项 | 说明 |
|--------|------|
| Service 测试 | 核心 Service 方法有单元测试（mock DAO） |
| API 测试 | 新端点有集成测试（happy path + 主要 error path） |
| AC 覆盖 | spec 中每条 AC 都有对应测试或手动验证 |
| 错误路径 | 权限拒绝、参数校验失败等错误路径有测试 |
| 测试质量 | mock 合理，不 mock 掉核心逻辑 |

> **务实适配**：当前测试基础薄弱，降低阈值但要求核心 Service 方法必须有测试。
> 前端暂用手动验证替代（tasks.md 中有「手动验证」描述即可）。

### 维度 6：代码风格

| 检查项 | 说明 |
|--------|------|
| DDD 分层 | 新代码在正确的层级（domain/services vs api/endpoints） |
| 命名一致 | DAO/Service/错误码命名遵循项目约定 |
| 代码重复 | 无复制粘贴式重复逻辑（应提取到 Service 或工具函数） |
| 未使用代码 | 无 dead code、注释掉的代码块、空函数 |
| 格式化 | Python 代码通过 ruff check（hook 自动处理） |

### 维度 7：文档同步（design.md 现状快照）

> **目的**：确保 feature 合入后，新 agent 接手时读 design.md 能 5 分钟建立准确认知 —— 没有过期描述、没有缺失的反直觉坑、没有未声明的对外契约。

| 检查项 | 说明 | 严重度 |
|--------|------|--------|
| design.md 存在 | feature 目录下有 `design.md`（按 `features/_templates/design.md` 起的稿） | HIGH（缺失直接 NEEDS_FIX） |
| 决策同步 | diff 中新增/替换的关键技术决策（数据格式、配对/匹配策略、同步 vs 异步、二进制依赖等）在 §3 方案对比有记录，且给出"何时该重新考虑" | HIGH |
| 数据流/契约同步 | diff 触及 API 路径 / 请求响应字段 / 内部 Service 入参出参 / 数据库新表新字段 / 关键文件职责变化 → §4.1 数据流、§4.2 字段约定、§4.3 模块职责对应章节已更新 | HIGH |
| 已知坑同步 | 修了一个"代码里看不出的"反直觉 bug（典型：上游字段格式不符合直觉、运行时值与文档不符、必须的兜底逻辑）→ §5 已知坑 表新增一行（带"如果不知道会怎样"+"在哪处理"） | HIGH |
| 契约/依赖同步 | 新增对外 endpoint / 新依赖 chat/knowledge/permission 等模块的隐式契约 / 新增系统二进制依赖 → §6 Outgoing / Incoming 已补 | HIGH |
| 修订历史 | §修订历史 末尾追加了本次 feature 完成的条目（日期 + 改动 + 触发原因） | MEDIUM |
| 与 spec 不冲突 | design.md 对当前实现的描述未与 spec.md AC 矛盾（spec 是不变目标，design 是当前实现，二者口径必须对齐） | HIGH |

**判定要点**：
- 若 design.md 与代码现状偏离 → 必须修复（视为 HIGH），不能合入
- 仅本地小修小补（不改变对外行为、不引入新坑、不动决策）→ 本维度全 PASS 即可
- design.md 不存在但已在 SDD 流程要求之内 → HIGH，要求按模板补全后再审

---

## 判定规则

| 结果 | 条件 | 动作 |
|------|------|------|
| **PASS** | 无 HIGH 或 MEDIUM | 可合并 |
| **PASS_WITH_WARNINGS** | 仅 MEDIUM 级 | 可合并，记录待改进 |
| **NEEDS_FIX** | 有 HIGH 级 | 修复后重审（最多 2 轮） |

---

## 输出格式

```markdown
# Code Review Report

**Feature**: <feature_name>
**Review scope**: <描述>
**Base branch**: 2.5.0-PM
**Changed files**: <数量>

## Summary

| Dimension | High | Medium | Low | Status |
|-----------|------|--------|-----|--------|
| Boundary Conditions | 0 | 0 | 0 | PASS |
| Permission & Auth | 0 | 0 | 0 | PASS |
| Concurrency Safety | 0 | 0 | 0 | PASS |
| Information Leakage | 0 | 0 | 0 | PASS |
| Test Coverage | 0 | 0 | 0 | PASS |
| Code Style | 0 | 0 | 0 | PASS |
| Docs Sync (design.md) | 0 | 0 | 0 | PASS |

## Findings（如有）

### HIGH
- [Permission] `xxx_endpoint.py:42` — 缺少 PermissionService.check() 调用

### MEDIUM
- [Style] `xxx_service.py:18` — DAO 方法未使用 @classmethod

## Overall: PASS / PASS_WITH_WARNINGS / NEEDS_FIX
```

More agent context in dataelement/bisheng

15 other files this repository gives its agents.