CodeReviewPlaybookSkill code-review-playbook

代码审查手册

概览

这项技能提供了一个全面的框架，用于有效的代码审查，提高代码质量，共享知识，促进合作。无论您是提供反馈的审查者还是准备代码以供审查的作者，这个手册确保审查是全面、一致和建设性的。

何时使用这项技能：

• 审查拉取请求或合并请求
• 准备代码以供审查（自我审查）
• 为团队建立代码审查标准
• 培训新开发人员审查最佳实践
• 解决关于代码质量的分歧
• 提高审查流程和效率

代码审查哲学

代码审查的目的

代码审查有多重目的：

1. 质量保证：捕捉错误、逻辑错误和边缘情况
2. 知识共享：在团队中传播领域知识
3. 一致性：确保代码库遵循约定和模式
4. 指导：帮助开发人员提高技能
5. 集体所有：建立对代码的共同责任
6. 文档：为将来的参考创建讨论历史

原则

友善和尊重：

• 审查代码，而不是人
• 假设积极意图
• 表扬好的解决方案
• 建设性地提出反馈

具体且可操作：

• 指向具体的代码行
• 解释为什么需要改变
• 提出具体的改进建议
• 在有帮助时提供示例

平衡速度与彻底性：

• 争取及时反馈（<24小时）
• 不要急于进行关键审查
• 使用自动化进行常规检查
• 将人工审查集中在逻辑和设计上

区分必须修复与希望有的：

• 使用传统评论来表示严重性
• 仅对关键问题阻塞合并
• 允许作者推迟次要改进
• 在后续票证中捕获推迟的工作

---

传统评论

标准化的审查评论格式，使意图清晰。

格式

<label> [decorations]: <subject>

[discussion]

标签

标签	含义	阻塞合并？
praise	突出积极的事情	否
nitpick	次要的，可选的建议	否
suggestion	提出改进	否
issue	应该解决的问题	通常
question	请求澄清	否
thought	考虑的想法	否
chore	例行任务（格式化，依赖项）	否
note	信息性评论	否
todo	需要后续工作的	也许
security	安全问题	是
bug	潜在的错误	是
breaking	破坏性变更	是

装饰

方括号中的可选修饰符：

装饰	含义
[blocking]	必须在合并前解决
[non-blocking]	可选的，可以推迟
[if-minor]	仅如果是快速修复

示例

// ✅ 好的：清晰、具体、可操作

praise: TypeScript泛型使用得非常好！

这使得函数更具可重用性，同时保持类型安全。

---

nitpick [non-blocking]: 考虑使用const而不是let

这个变量从未重新分配，所以`const`更合适：
```typescript
const MAX_RETRIES = 3;

---

issue: API调用缺少错误处理

如果API返回500错误，这将使应用程序崩溃。 添加try/catch块：

try {
  const data = await fetchUser(userId);
  // ...
} catch (error) {
  logger.error('Failed to fetch user', { userId, error });
  throw new UserNotFoundError(userId);
}

---

question: 为什么在这里使用Map而不是对象？

是否有特定的原因选择这种数据结构？ 如果是出于性能考虑，可以添加注释解释吗？

---

security [blocking]: API端点未认证

/api/admin/users端点缺少认证中间件。 这允许未经认证的访问敏感用户数据。

添加auth中间件：

router.get('/api/admin/users', requireAdmin, getUsers);

---

suggestion [if-minor]: 提取魔术数字到命名常量

考虑提取这个值：

const CACHE_TTL_SECONDS = 3600;
cache.set(key, value, CACHE_TTL_SECONDS);

---

## 审查流程

### 1. 审查前

**检查上下文：**
- 阅读PR/MR描述
- 理解目的和范围
- 查看相关票证或问题
- 检查CI/CD管道状态

**验证自动化检查：**
- [ ] 测试通过
- [ ] 没有linting错误
- [ ] 类型检查通过
- [ ] 代码覆盖率达标
- [ ] 没有合并冲突

**留出时间：**
- 小型PR（<200行）：15-30分钟
- 中型PR（200-500行）：30-60分钟
- 大型PR（>500行）：1-2小时（或要求拆分）

### 2. 审查中

**遵循模式：**

1. **高层审查**（5-10分钟）
   - 阅读PR描述并理解意图
   - 浏览所有更改的文件以获得概览
   - 验证方法在架构上是否合理
   - 检查更改是否符合声明的目的

2. **详细审查**（20-45分钟）
   - 逐行代码审查
   - 检查逻辑、边缘情况、错误处理
   - 验证测试是否覆盖新代码
   - 查找安全漏洞
   - 确保代码遵循团队约定

3. **测试考虑**（5-10分钟）
   - 测试是否全面？
   - 测试是否测试了正确的事情？
   - 是否涵盖了边缘情况？
   - 测试数据是否真实？

4. **文档检查**（5分钟）
   - 复杂部分是否有注释？
   - 公共API是否有文档？
   - 是否注明了破坏性变更？
   - 如果需要，README是否已更新？

### 3. 审查后

**提供明确的决定：**
- ✅ **批准**：代码已准备好合并
- 💬 **评论**：已提供反馈，无需采取行动
- 🔄 **请求更改**：在合并前必须解决的问题

**回应作者：**
- 及时回答问题
- 在更改后重新审查
- 在问题解决后批准
- 感谢作者解决反馈

---

## 审查清单

### 通用代码质量

- [ ] **可读性**：代码易于理解
- [ ] **命名**：变量和函数有清晰、描述性的名称
- [ ] **注释**：复杂逻辑已解释
- [ ] **格式化**：代码遵循团队风格指南
- [ ] **DRY**：没有不必要的重复
- [ ] **SOLID原则**：代码适用SOLID原则
- [ ] **函数大小**：函数专注且<50行
- [ ] **圈复杂度**：函数复杂度<10

### 功能

- [ ] **正确性**：代码按预期工作
- [ ] **边缘情况**：边界条件处理（null、empty、min/max）
- [ ] **错误处理**：错误被捕获并适当处理
- [ ] **日志记录**：适当的日志级别和消息
- [ ] **输入验证**：用户输入已验证和清理
- [ ] **输出验证**：响应与预期模式匹配

### 测试

- [ ] **测试覆盖率**：新代码有测试
- [ ] **测试质量**：测试实际上测试了正确的事情
- [ ] **边缘情况测试**：测试覆盖边界条件
- [ ] **错误路径测试**：测试了错误处理
- [ ] **测试隔离**：测试不依赖于彼此
- [ ] **测试命名**：测试名称描述了正在测试的内容

### 性能

- [ ] **数据库查询**：避免N+1查询
- [ ] **缓存**：使用适当的缓存
- [ ] **算法效率**：没有不必要的慢算法（当O(n)可能时，没有O(n²)）
- [ ] **资源清理**：文件、连接、内存已释放
- [ ] **懒加载**：尽可能推迟重操作

### 安全

- [ ] **认证**：受保护的端点需要认证
- [ ] **授权**：用户只能访问自己的数据
- [ ] **输入清理**：防止SQL注入、XSS（输入清理、CSP头）
- [ ] **秘密管理**：没有硬编码的凭据或API密钥
- [ ] **加密**：敏感数据在休息和传输时加密
- [ ] **仅HTTPS**：生产流量使用HTTPS
- [ ] **速率限制**：端点受到滥用保护

### 语言特定（JavaScript/TypeScript）

- [ ] **Async/Await**：正确处理Promise
- [ ] **类型安全**：TypeScript类型具体（不是`any`）
- [ ] **空值性**：需要时进行空值检查（`?.`操作符）
- [ ] **数组方法**：适当使用map/filter/reduce
- [ ] **Const与Let**：使用const表示不可变值
- [ ] **箭头函数**：适当使用箭头与常规函数

### 语言特定（Python）

- [ ] **PEP 8**：遵循Python风格指南
- [ ] **类型提示**：函数有类型注释
- [ ] **列表推导**：适当使用（不过度使用）
- [ ] **上下文管理器**：使用`with`处理文件/连接处理
- [ ] **异常处理**：捕获特定异常（不是裸`except:`）
- [ ] **F-Strings**：使用现代字符串格式化

### API设计

- [ ] **RESTful**：遵循REST约定（如果是REST API）
- [ ] **一致命名**：端点遵循命名模式
- [ ] **HTTP方法**：使用正确的方法（GET、POST、PUT、DELETE）
- [ ] **状态码**：返回适当的HTTP状态码
- [ ] **错误响应**：一致的错误格式
- [ ] **分页**：大型列表分页
- [ ] **版本控制**：遵循API版本策略

### 数据库

- [ ] **迁移**：数据库更改有迁移
- [ ] **索引**：创建适当的索引
- [ ] **事务**：维护ACID属性
- [ ] **级联**：正确处理删除级联
- [ ] **约束**：定义外键、唯一约束
- [ ] **N+1查询**：需要时使用急切加载

### 文档

- [ ] **PR描述**：清楚地解释更改
- [ ] **代码注释**：解释复杂逻辑
- [ ] **API文档**：公共API有文档（JSDoc、docstrings）
- [ ] **README**：如果功能更改，则更新
- [ ] **CHANGELOG**：记录破坏性变更

---

## 审查反馈模式

### 如何提供建设性反馈

#### ✅ 好的反馈

issue: 这个函数没有处理用户为空的情况

如果getUserById()返回null（未找到用户），这将抛出： Cannot read property 'email' of null

添加空值检查：

const user = await getUserById(userId);
if (!user) {
  throw new UserNotFoundError(userId);
}
return user.email;

**为什么好：**
- 具体（指出确切问题）
- 解释影响（会发生什么）
- 提出具体解决方案
- 提供示例代码

#### ❌ 坏反馈

This is wrong. Fix it.

**为什么不好：**
- 不具体（什么错了？）
- 没有帮助（如何修复？）
- 听起来严厉（不建设性）

---

### 如何接受反馈

**作为作者：**
1. **假设良好意图**：审查者试图提供帮助
2. **提问**：如果反馈不清晰，要求澄清
3. **承认有效点**：接受反馈
4. **解释你的理由**：如果你不同意，解释为什么
5. **及时做出更改**：快速解决问题
6. **说谢谢**：感谢审查者的时间

**回应反馈：**

```markdown
✅ 好的回应：
> 好主意！我没有考虑到空值情况。
> 我在提交abc123中添加了空值检查。

✅ 好的回应（不同意）：
> 我在这里选择Map，是因为我们需要在大型数据集上进行O(1)查找。
> 对象也可以工作，但在规模上表现更差（n > 10,000）。
> 我添加了一条注释来解释这种权衡。

❌ 坏回应：
> 这工作得很好。不改了。

---

代码审查反模式

对于审查者

❌ 没有明确价值的挑剔

nitpick: 在这里加一个空格
nitpick: 将这个变量重命名为`userInfo`而不是`userData`
nitpick: 将这个导入移到顶部

更好： 使用自动化工具（Prettier、linters）进行格式化。

❌ 在不理解上下文的情况下逐行审查 更好： 首先阅读PR描述，了解大局。

❌ 基于个人喜好阻塞

这应该是一个类，而不是函数。

更好： 只对客观问题（错误、安全）阻塞。分别讨论偏好。

❌ 在评论中重写代码 更好： 提出改进，不要提供完整实现（除非非常有帮助）。

❌ 审查疲劳（未经阅读就批准） 更好： 如果你没有时间，说出来。不要橡皮图章。

对于作者

❌ 巨型拉取请求（>1000行） 更好： 拆分成更小、更集中的PR。

❌ 没有描述 更好： 用上下文和测试说明写清晰的PR描述。

❌ 提交失败的测试 更好： 在请求审查之前确保所有自动化检查通过。

❌ 对反馈采取防御态度 更好： 优雅地接受反馈，尊重地讨论。

❌ 审查后强制推送 更好： 添加新提交，以便审查者可以看到变化。

---

审查模板

标准PR模板

## 描述

简要总结更改内容及原因。

修复#[问题编号]

## 变更类型

- [ ] 错误修复（不破坏更改，修复问题）
- [ ] 新功能（不破坏更改，增加功能）
- [ ] 破坏性更改（修复或功能会导致现有功能无法按预期工作）
- [ ] 重构（无功能更改）
- [ ] 文档更新

## 这个是如何测试的？

- [ ] 单元测试已添加/更新
- [ ] 集成测试已添加/更新
- [ ] 在本地环境中手动测试
- [ ] 在暂存环境中测试

## 清单

- [ ] 我的代码遵循了本项目的样式指南
- [ ] 我已经对代码进行了自我审查
- [ ] 我在难以理解的区域添加了注释
- [ ] 我已经对文档进行了相应的更改
- [ ] 我的更改没有产生新的警告
- [ ] 我已经添加了测试，证明我的修复是有效的，或者我的功能是工作的
- [ ] 新增和现有的单元测试在本地通过我的更改
- [ ] 任何依赖的更改已经合并并发布

## 截图（如果适用）

[为UI更改添加截图]

## 额外说明

[审查者需要的任何额外上下文或说明]

安全审查模板

## 安全审查清单

### 认证与授权
- [ ] 端点需要适当的认证
- [ ] 在行动前检查用户权限
- [ ] JWT令牌已验证且未过期
- [ ] 会话管理是安全的

### 输入验证
- [ ] 所有用户输入都已验证
- [ ] 防止SQL注入（参数化查询）
- [ ] 防止XSS（输入清理，CSP头）
- [ ] 对于状态更改操作，有CSRF保护

### 数据保护
- [ ] 敏感数据在休息时加密
- [ ] 数据在传输时使用TLS/HTTPS
- [ ] 没有硬编码的API密钥和秘密
- [ ] PII得到适当处理（GDPR/CCPA合规）

### 依赖项
- [ ] 依赖项中没有已知漏洞（npm audit / pip-audit）
- [ ] 依赖项来自可信来源
- [ ] 依赖项版本已固定

### 日志记录与监控
- [ ] 不记录敏感数据（密码、令牌）
- [ ] 安全事件已记录（认证失败尝试）
- [ ] 公共端点有速率限制

**已识别的安全问题：**
[列出发现的任何安全问题]

**审查者签名：**
[姓名，日期]

---

审查指标和目标

健康的审查指标

指标	目标	目的
审查时间	<24小时	快速反馈循环
PR大小	<400行	可管理的审查
首次审查批准率	20-30%	平衡速度与质量
每PR评论数	3-10	参与但不压倒性
来回讨论轮数	1-2	高效沟通
批准后合并时间	<2小时	避免陈旧分支

警告信号

• ⚠️ PR超过3天未审查（审查能力问题）
• ⚠️ 首次审查的批准率超过90%（橡皮图章）
• ⚠️ 平均PR大小超过800行（PR过大）
• ⚠️ 每PR超过15条评论（过于挑剔或要求不明确）
• ⚠️ 超过5轮审查（沟通不畅或标准不明确）

---

与代理集成

代码质量审查代理

• 在审查代码时使用这个手册
• 应用传统评论格式
• 遵循语言特定清单
• 提供结构化、可操作的反馈

后端系统架构师

• 审查架构合理性
• 检查设计模式和可扩展性
• 验证API设计是否符合最佳实践

前端UI开发人员

• 审查组件结构和模式
• 检查可访问性和响应式设计
• 验证UI/UX实现

安全审查者（未来代理）

• 专注于安全清单
• 识别漏洞
• 验证合规性要求

---

高级模式检测（Opus 4.5）

复杂审查的扩展思考

对于大型PR或系统分析，利用Opus 4.5的扩展思考进行深度模式检测：

何时使用扩展思考：

• PR涉及>10个文件跨多个模块
• 审查安全关键代码路径
• 分析架构一致性
• 检测跨文件代码异味
• 评估破坏性变更影响

扩展思考审查模式：

基于Anthropic的扩展思考API：

import Anthropic from '@anthropic-ai/sdk';

interface ReviewResult {
  issues: ReviewIssue[];
  patterns: DetectedPattern[];
  recommendations: string[];
  riskScore: number;
}

async function performDeepCodeReview(
  prDiff: string,
  codebaseContext: string
): Promise<ReviewResult> {
  const anthropic = new Anthropic();

  // 扩展思考需要budget_tokens < max_tokens
  // 最低预算：1,024个token
  const response = await anthropic.messages.create({
    model: 'claude-opus-4-5-20251101',
    max_tokens: 16000,
    thinking: {
      type: 'enabled',
      budget_tokens: 8000 // 复杂审查需要深度分析
    },
    messages: [{
      role: 'user',
      content: `
        执行全面的代码审查分析：

        ## PR更改
        ${prDiff}

        ## 代码库上下文
        ${codebaseContext}

        ## 分析要求
        1. **系统模式检测**：识别重复模式（好的和坏的）
        2. **跨文件影响**：追踪更改如何影响其他模块
        3. **安全分析**：深度扫描漏洞（OWASP Top 10）
        4. **架构一致性**：验证与现有模式的对齐
        5. **技术债务评估**：识别增加或解决的债务

        提供结构化输出：
        - 临界问题（必须修复）
        - 警告（应该修复）
        - 建议（希望有）
        - 检测到的模式（积极和消极）
        - 风险评分（1-10）
      `
    }]
  });

  // 响应包含思考块，后跟文本块
  // content: [{ type: 'thinking', thinking: '...' }, { type: 'text', text: '...' }]
  return parseReviewResponse(response);
}

系统代码异味检测

检测跨多个文件的模式：

interface CodeSmellPattern {
  type: 'duplication' | 'coupling' | 'complexity' | 'naming' | 'architecture';
  severity: 'critical' | 'warning' | 'info';
  locations: FileLocation[];
  description: string;
  suggestion: string;
}

const SMELL_PATTERNS = {
  // 跨文件重复
  duplication: {
    triggers: ['3+文件中的类似逻辑', '复制粘贴模式', '重复的错误处理'],
    action: '提取到共享实用程序或基类'
  },

  // 紧密耦合
  coupling: {
    triggers: ['循环导入', '神对象', '特征羡慕'],
    action: '应用依赖注入或接口隔离'
  },

  // 复杂性增加
  complexity: {
    triggers: ['>3级的嵌套回调', '函数>50行', '圈复杂度>10'],
    action: '分解为更小、更专注的函数'
  },

  // 不一致的命名
  naming: {
    triggers: ['混合约定', '不清晰的缩写', '不一致的复数化'],
    action: '与代码库命名约定对齐'
  },

  // 架构漂移
  architecture: {
    triggers: ['层违规', '缺失抽象', '不适当的亲密度'],
    action: '重构以遵循既定的架构模式'
  }
};

安全深度分析

扩展思考使全面的安全审查成为可能：

interface SecurityFinding {
  category: 'injection' | 'auth' | 'crypto' | 'exposure' | 'config';
  severity: 'critical' | 'high' | 'medium' | 'low';
  cwe: string; // CWE ID
  location: FileLocation;
  description: string;
  remediation: string;
}

async function performSecurityReview(
  code: string,
  context: SecurityContext
): Promise<SecurityFinding[]> {
  const anthropic = new Anthropic();

  const response = await anthropic.messages.create({
    model: 'claude-opus-4-5-20251101',
    max_tokens: 12000,
    thinking: {
      type: 'enabled',
      budget_tokens: 6000 // 安全分析需要深度推理
    },
    messages: [{
      role: 'user',
      content: `
        对这段代码进行安全分析：

        ${code}

        上下文：
        - 语言：${context.language}
        - 框架：${context.framework}
        - 曝光：${context.isPublicFacing ? '公共' : '内部'}

        检查：
        1. 注入漏洞（SQL、XSS、命令）
        2. 认证/授权缺陷
        3. 加密弱点
        4. 敏感数据曝光
        5. 安全配置错误

        对于每个发现，提供CWE ID和具体补救措施。
      `
    }]
  });

  return parseSecurityFindings(response);
}

跨文件影响分析

追踪更改如何在整个代码库中传播：

interface ImpactAnalysis {
  directImpact: FileImpact[];
  indirectImpact: FileImpact[];
  breakingChanges: BreakingChange[];
  testCoverage: TestCoverageGap[];
}

async function analyzeChangeImpact(
  changedFiles: string[],
  dependencyGraph: DependencyGraph
): Promise<ImpactAnalysis> {
  // 构建影响图
  const directlyAffected = new Set<string>();
  const indirectlyAffected = new Set<string>();

  for (const file of changedFiles) {
    // 导入此文件的文件
    const dependents = dependencyGraph.getDependents(file);
    dependents.forEach(d => directlyAffected.add(d));

    // 第二级依赖
    for (const dependent of dependents) {
      const secondLevel = dependencyGraph.getDependents(dependent);
      secondLevel.forEach(d => {
        if (!directlyAffected.has(d)) {
          indirectlyAffected.add(d);
        }
      });
    }
  }

  // 使用扩展思考进行破坏性变更分析
  const breakingAnalysis = await analyzeBreakingChanges(
    changedFiles,
    Array.from(directlyAffected)
  );

  return {
    directImpact: Array.from(directlyAffected).map(f => ({ file: f, type: 'direct' })),
    indirectImpact: Array.from(indirectlyAffected).map(f => ({ file: f, type: 'indirect' })),
    breakingChanges: breakingAnalysis.breaking,
    testCoverage: breakingAnalysis.gaps
  };
}

审查自动化与模型分层

通过智能模型选择优化审查成本：

type ModelTier = 'opus' | 'sonnet' | 'haiku';

interface ReviewConfig {
  model: ModelTier;
  thinkingBudget?: number;
  focus: string[];
}

function selectReviewModel(pr: PullRequest): ReviewConfig {
  // 临界路径 - 使用Opus与扩展思考
  if (pr.touchesCriticalPath || pr.isSecurityRelated) {
    return {
      model: 'opus',
      thinkingBudget: 8000,
      focus: ['security', 'architecture', 'breaking-changes']
    };
  }

  // 大型PR - 使用Opus进行系统分析
  if (pr.filesChanged > 10 || pr.linesChanged > 500) {
    return {
      model: 'opus',
      thinkingBudget: 5000,
      focus: ['patterns', 'impact', 'consistency']
    };
  }

  // 标准PR - Sonnet用于彻底审查
  if (pr.linesChanged > 100) {
    return {
      model: 'sonnet',
      focus: ['correctness', 'style', 'tests']
    };
  }

  // 小型更改 - Haiku用于快速审查
  return {
    model: 'haiku',
    focus: ['correctness', 'style']
  };
}

生成审查评论

自动生成传统评论：

interface GeneratedComment {
  label: 'praise' | 'nitpick' | 'suggestion' | 'issue' | 'question' | 'security' | 'bug';
  decoration?: 'blocking' | 'non-blocking' | 'if-minor';
  subject: string;
  discussion: string;
  location: { file: string; line: number };
}

function formatConventionalComment(comment: GeneratedComment): string {
  const decoration = comment.decoration ? ` [${comment.decoration}]` : '';
  return `${comment.label}${decoration}: ${comment.subject}

${comment.discussion}`;
}

// 示例输出：
// security [blocking]: 用户搜索中的SQL注入漏洞
//
// 搜索查询是使用字符串连接构建的：
// ```typescript
// const query = `SELECT * FROM users WHERE name = '${searchTerm}'`;
// ```
//
// 使用参数化查询代替：
// ```typescript
// const query = 'SELECT * FROM users WHERE name = $1';
// const result = await db.query(query, [searchTerm]);
// ```

---

快速入门指南

对于审查者：

1. 阅读PR描述并理解意图
2. 检查自动化检查是否通过
3. 进行高层审查（架构、方法）
4. 进行详细审查（逻辑、边缘情况、测试）
5. 使用传统评论进行清晰沟通
6. 提供决定：批准、评论或请求更改

对于作者：

1. 写清晰的PR描述
2. 在请求审查之前进行自我审查
3. 确保所有自动化检查通过
4. 保持PR专注且合理大小（<400行）
5. 及时且尊重地回应反馈
6. 进行请求的更改或解释理由

---

技能版本：2.0.0 最后更新：2025-11-27 维护者：AI代理中心团队