John Qiu
7eed2b8f10
Karpathy 四原则融合到 req 技能工作流 (REQ-20260421-0003): - dev-coding: 新增 Step 0「验证优先」(Goal-Driven Execution) - dev-review: 五视角 → 六视角，新增 Scope 审计者 (Simplicity + Surgical) - review-checklist/general: 新增 Karpathy 反模式速查表 - karpathy-guidelines-plugin: 新增独立插件，含四原则全文 + 与 req 工作流映射
306 lines
11 KiB
Markdown
306 lines
11 KiB
Markdown
---
|
||
name: dev-review
|
||
description: 代码评审技能。六视角对抗性扫描法（含 Karpathy Scope 审计），用于 PR 代码审查、安全评审、质量检查。当执行 /req cr 或独立 PR review 时自动激活。
|
||
---
|
||
|
||
# 代码评审 Skill (dev-review)
|
||
|
||
## 概述
|
||
|
||
独立的代码评审技能，核心方法论是**六视角对抗性扫描法**（五个传统安全/健壮性视角 + Karpathy Scope 审计视角）。
|
||
|
||
**适用场景**：
|
||
- `/req cr [REQ-ID]` — 需求流程中的代码评审阶段
|
||
- 独立 PR review — 不绑定需求的代码审查
|
||
- 安全评审 — 专项安全扫描
|
||
|
||
**核心原则**：实现阶段关注"怎么让它跑通"，评审阶段关注**"怎么让它出错"**。AI 必须切换到对抗性思维。
|
||
|
||
---
|
||
|
||
## 技能间契约
|
||
|
||
| 上游 | 本技能输入 | 本技能输出 | 下游 |
|
||
|------|-----------|-----------|------|
|
||
| dev-coding | PR diff + 开发设计文档 | CR 报告（六视角扫描 + 发现汇总 + 结论） | dev-test |
|
||
|
||
---
|
||
|
||
## 工作流程
|
||
|
||
```
|
||
1. 确定评审范围
|
||
├── git diff 获取变更文件列表
|
||
├── 统计文件数、行数
|
||
└── 排除 test 文件（单独审查）
|
||
|
||
2. 读取变更代码
|
||
├── 逐个读取变更<E58F98><E69BB4><EFBFBD>件源码
|
||
├── 理解业务上下文
|
||
└── 参考开发设计文档（如有）
|
||
|
||
3. 五视角扫描（核心）
|
||
├── 攻击<E694BB><E587BB><EFBFBD>视角
|
||
├── 泄露者视角
|
||
├── 并发者视角
|
||
├── 边界者视角
|
||
└── 依赖者视角
|
||
|
||
4. 加载项目检查清单（如有）
|
||
└── review-checklist 插件
|
||
|
||
5. 生成 CR 报告
|
||
├── 变更概要
|
||
├── 五视角扫描结果
|
||
├── 发现汇总表
|
||
└── 结论（通过/有条件通过/需修改）
|
||
|
||
6. 创建评审任务（req 流程内）
|
||
├── ai-proj task create【代码评审】
|
||
├── 关联需求（linkRole=code_review）
|
||
└── 附加 CR 报告文档
|
||
|
||
7. 处理发现
|
||
├── Critical/High → 创建修复任务
|
||
└── Medium/Low → 记录建议
|
||
```
|
||
|
||
---
|
||
|
||
## 六视角对抗性扫描法
|
||
|
||
### 总览
|
||
|
||
| 视角 | 思维模式 | 核心问题 |
|
||
|------|---------|---------|
|
||
| **1. 攻击者** | "我怎么绕过/<2F><><EFBFBD>用它？" | 跨租户泄露、越权访问、参数注入、重放攻击 |
|
||
| **2. 泄露者** | "它暴露了什么不该暴露的？" | 错误信息泄露、日志敏感数据、响应内部细节 |
|
||
| **3. 并发者** | "两个请求同时来会怎样？" | 竞态条件、双重扣款、幂等性缺失、锁粒度 |
|
||
| **4. 边界者** | "极端输入会怎样？" | 空值/零值/负值/超长字符串、类型溢出、分页越界 |
|
||
| **5. 依赖者** | "外部服务挂了会怎样？" | 超时处理、重试策略、降级方案、连接泄露 |
|
||
|
||
### 视角1：攻击者（多租户安全）
|
||
|
||
**思维模式**：我是恶意用户，如何绕过权限获取他人数据。
|
||
|
||
扫描清单：
|
||
- [ ] 所有 Store/Repository 层查询是否带 `tenant_id` 过滤？
|
||
- [ ] 通过 ID 直接查询的方法是否校验归属？
|
||
- [ ] 用户只能操作自己的数据？（consumer_id 校验）
|
||
- [ ] URL/请求参数是否有注入风险？（SQL、URL、命令注入）
|
||
- [ ] 外部输入是否直接拼接到查询/URL？（应使用参数化查询或编码）
|
||
- [ ] 批量操作是否逐条校验权限？（不能只校验第一条）
|
||
- [ ] 文件上传是否有类型/大小限制？
|
||
|
||
**典型发现示例**：
|
||
```
|
||
file:line — Store.GetByID(id) 缺少 tenant_id 过滤，
|
||
攻击者可通过遍<EFBFBD><EFBFBD> ID 获取其他租户数据。
|
||
建议：添加 WHERE tenant_id = ? 条件。
|
||
```
|
||
|
||
### 视角2：泄露者（信息安全）
|
||
|
||
**思维模式**：我是安全审计员，检查每个出口是否泄露了不该泄露的信息。
|
||
|
||
扫描清单：
|
||
- [ ] 错误消息是否泄露业务状态？（如"手机号未注册"暴露用户存在性）
|
||
- [ ] 日志是否打印了密码、token、密钥、身份证号？
|
||
- [ ] 响应是否包含不必要的内部字段？（如内部 ID、数据库字段名、堆栈跟踪）
|
||
- [ ] panic recover 后是否返回了内部错误详情？
|
||
- [ ] 导出/下载功能是否过滤了敏感字段？
|
||
|
||
### 视角3：并发者（数据一致性）
|
||
|
||
**思维模式**：两个用户同时操作同一条数据，会发生什么。
|
||
|
||
扫描清单：
|
||
- [ ] 涉及金额/库存变更是否使用事务 + 悲观锁/乐观锁？
|
||
- [ ] 关键操作是否有幂等保护？（bizNo 唯一索引、幂等键）
|
||
- [ ] 全局状态（如进程内计数器、缓存）重启后是否安全？
|
||
- [ ] 是否有 TOCTOU（检查-使用）竞态？（先查状态再操作，中间被修改）
|
||
- [ ] 并发创建是否会产生重复数据？（唯一约束）
|
||
|
||
### 视角4：边界者（健壮性）
|
||
|
||
**思维模<E7BBB4><E6A8A1>**：用最极端的输入来测试系统的承受能力。
|
||
|
||
扫描清单：
|
||
- [ ] 必填参数是否有 `binding:"required"` 校验？
|
||
- [ ] 数值参数是否有范围校验？（min/max，防止负数、溢出）
|
||
- [ ] 字符串是否有长度限制？（防止超长输入消耗内存）
|
||
- [ ] 分页参数是否有默认值和上限？（page_size 不能为 0 或 999999）
|
||
- [ ] 数组参数是否有长度限制？（批量操作不能传 10 万条）
|
||
- [ ] 空数组/空字符串是否正确处理？（不应触发不必要的数据库操作）
|
||
- [ ] 除零错误？百分比计算分母为 0？
|
||
|
||
### 视角5：依赖者（可靠性）
|
||
|
||
**思维模式**：外部服务全部挂掉，系统还能正常运行吗。
|
||
|
||
扫描清单：
|
||
- [ ] HTTP 客户端是否设置超时？（connect/read/write timeout）
|
||
- [ ] 外部 API 调用失败是否有合理的错误处理？（不能直接 panic）
|
||
- [ ] 是否有重试策略？重试是否有退避？是否幂等安全？
|
||
- [ ] 数据库连接池配置是否合理？（max idle/max open/lifetime）
|
||
- [ ] Redis 不可用时是否有降级方案？（缓存穿透到数据库）
|
||
- [ ] token 过期/刷新逻辑是否正确？（access vs refresh 不同策略）
|
||
|
||
### 视角6：Scope 审计者（Karpathy: Simplicity + Surgical）
|
||
|
||
**思维模式**：每一行变更，需求有没有要求它？
|
||
|
||
> "Touch only what you must. Clean up only your own mess."
|
||
> "Every changed line should trace directly to the user's request."
|
||
|
||
扫描清单：
|
||
- [ ] diff 中变更的**每个文件**，是否都在 req-design 变更文件清单中？（超出清单 = 疑似顺手重构）
|
||
- [ ] 新增的函数/方法/结构体，每个都有对应 AC 需要它？
|
||
- [ ] 是否引入了"未来可能用到"的参数、配置项、可选字段、接口抽象？
|
||
- [ ] 是否修改了本次 AC 无关的注释、格式、变量名、import 顺序？
|
||
- [ ] 代码量是否合理？实现简单 AC 超过 200 行须说明必要性
|
||
（"If you write 200 lines and it could be 50, rewrite it"）
|
||
- [ ] 错误处理是否只覆盖真实会发生的场景？不为不可能的情况写防御代码
|
||
|
||
**典型发现示例**：
|
||
```
|
||
backend/services/user_service.go:45 — 新增了 WithRetry 参数，但 AC 中无重试需求。
|
||
建议：移除该参数，AC 有需要时再添加。严重度：Low
|
||
```
|
||
|
||
---
|
||
|
||
## CR 报告模板
|
||
|
||
```markdown
|
||
## 代<><E4BBA3>评审报告 — {需求标题/PR 标题}
|
||
|
||
**日期**: YYYY-MM-DD
|
||
**评审范围**: {N} 个文件, {M} 行变更
|
||
**评审人**: AI (dev-review)
|
||
|
||
### 变更概要
|
||
{1-3 句描述本次变更的目的和范围}
|
||
|
||
### 六视角扫描结果
|
||
|
||
#### 1. 攻击者视角
|
||
{扫描发现，或 "未发现问题"}
|
||
|
||
#### 2. 泄露者视角
|
||
{扫描发现，或 "未发现问题"}
|
||
|
||
#### 3. 并发者视角
|
||
{扫描发现，或 "未发现问题"}
|
||
|
||
#### 4. 边界者视角
|
||
{扫描发现，或 "未发现问题"}
|
||
|
||
#### 5. 依赖者视角
|
||
{扫描发现，或 "未发现问题"}
|
||
|
||
#### 6. Scope 审计者视角（Karpathy）
|
||
{扫描发现，或 "所有变更文件均在设计清单范围内，无过度实现"}
|
||
|
||
### 审查发现汇总
|
||
|
||
| # | 严重度 | 文件:行号 | <20><><EFBFBD>角 | 描述 | 建议 |
|
||
|---|--------|----------|------|------|------|
|
||
| 1 | {Critical/High/Medium/Low} | {file:line} | {视角} | {问题} | {建议} |
|
||
|
||
### 统计
|
||
|
||
| 严重度 | 数量 |
|
||
|--------|------|
|
||
| Critical | 0 |
|
||
| High | 0 |
|
||
| Medium | 0 |
|
||
| Low | 0 |
|
||
|
||
### 结论
|
||
|
||
**{通过 / 有条件通过 / 需修改}**
|
||
|
||
{结论说明：如果有 Critical/High 必须修复后重新评审}
|
||
```
|
||
|
||
---
|
||
|
||
## 严重度定义
|
||
|
||
| 严重度 | 含义 | 处理方式 |
|
||
|--------|------|---------|
|
||
| **Critical** | 安全漏洞、数据泄露、资金风险 | 必须修复，阻断合并 |
|
||
| **High** | 数据一致性风险、业务逻辑错误 | 必须修复，阻断合并 |
|
||
| **Medium** | 边界未处理、缺少校验、性能隐患 | 建议修复，不阻断 |
|
||
| **Low** | 代码风格、命名优化、文档补充 | 可选修复 |
|
||
|
||
---
|
||
|
||
## CR 报告质量门禁
|
||
|
||
`/req next` 从 review 阶段推进时，检查 CR 报告质量：
|
||
|
||
| 检查项 | 标准 |
|
||
|--------|------|
|
||
| 文档存在 | CR 任务有附加文档 |
|
||
| 字数 | ≥ 500 字 |
|
||
| 代码引用 | 含 `file:line` 格式的引用 |
|
||
| 六视角扫描 | 含全部 6 个视角章节（含 Scope 审计者） |
|
||
| 结论章节 | 含明确的通过/不通过结论 |
|
||
|
||
---
|
||
|
||
## 插件支持
|
||
|
||
| 插件 | 触发条件 | 说明 |
|
||
|------|---------|------|
|
||
| `review-checklist` | 每次 CR | 加载项目特定检查清单 |
|
||
| `figma-design-qa` | 有设计稿 | 设计还原度对比 |
|
||
|
||
---
|
||
|
||
## 与 ai-proj 集成
|
||
|
||
### req 流程内（/req cr）
|
||
|
||
```typescript
|
||
// 1. 确认 implementation 任务已完成
|
||
mcp__ai-proj__get_requirement_tasks({ requirementId })
|
||
// 检查所有 linkRole=implementation 的任务状态
|
||
|
||
// 2. 创建 CR 任务
|
||
mcp__ai-proj__create_task({ title: "【代码评审】CR: {需求标题}" })
|
||
mcp__ai-proj__link_tasks_to_requirement({
|
||
requirementId, taskIds: [crTaskId], linkRole: "code_review"
|
||
})
|
||
|
||
// 3. 附加 CR 报告
|
||
mcp__ai-proj__create-and-attach({
|
||
taskId: crTaskId,
|
||
title: "代码评审报告",
|
||
content: "<CR <20><><EFBFBD>告 Markdown>"
|
||
})
|
||
|
||
// 4. High/Critical 发现 → 创建修复任务
|
||
mcp__ai-proj__create_task({ title: "【修复】{问题描述}" })
|
||
mcp__ai-proj__link_tasks_to_requirement({
|
||
requirementId, taskIds: [fixTaskId], linkRole: "implementation"
|
||
})
|
||
```
|
||
|
||
### 独立 PR review
|
||
|
||
不需要 ai-proj 集成，直接输出 CR 报告到对话。
|
||
|
||
---
|
||
|
||
## 最佳实践
|
||
|
||
1. **先理解再审查** — 读完所有变更<E58F98><E69BB4><EFBFBD>件后再开始扫描，避免断章取义
|
||
2. **对抗性思维** — 切换到"怎么让它出错"的心态，不是"怎么让它跑通"
|
||
3. **证据驱动** — 每个发现必须引用具体的 `file:line`
|
||
4. **严重度准确** — 不要所有问题都标 High，按实际影响分级
|
||
5. **建议可操作** — 每个发现必须附带具体修复建议
|
||
6. **关注变更** — 评审范围是 diff，不要对未变更的代码提意见（除非变更引入了对旧代码的新风险）
|