PR 评审专家

pr-review-expert
分类编程
作者Alireza Rezvani
许可MIT
评分4.60/5
使用15.0K

PR Review Expert (PR 审查专家)

等级： POWERFUL
类别： Engineering
领域： Code Review / Quality Assurance

---

概述

针对 GitHub PR 和 GitLab MR 提供结构化、系统化的代码审查。不仅限于代码风格的微调，该技能还可执行影响范围分析 (Blast Radius Analysis)、安全扫描、破坏性变更检测以及测试覆盖率增量计算。最终生成一份包含 30 多个检查项且结论优先级明确的审查报告。

---

核心能力

  • 影响范围分析 — 追踪哪些文件、服务和下游消费者可能会受到影响
  • 安全扫描 — SQL 注入、XSS、权限绕过、密钥泄露、依赖漏洞
  • 测试覆盖率增量 — 新代码与新测试的比例
  • 破坏性变更检测 — API 契约、数据库 Schema 迁移、配置键值
  • 票据关联 — 验证 Jira/Linear 票据是否存在且与范围匹配
  • 性能影响 — N+1 查询、Bundle 体积回退、内存分配

---

使用场景

  • 在合并任何涉及共享库、API 或数据库 Schema 的 PR/MR 之前
  • 当 PR 规模较大（变更超过 200 行）且需要结构化审查时
  • 为新贡献者提供详细反馈以帮助其上手
  • 涉及安全敏感的代码路径（鉴权、支付、PII 个人隐私数据处理）
  • 事故发生后 — 主动审查类似的 PR

---

获取 Diff

GitHub (gh CLI)

bash
# 在终端查看 diff
gh pr diff <PR_NUMBER>

获取 PR 元数据（标题、正文、标签、关联 issue）

gh pr view <PR_NUMBER> --json title,body,labels,assignees,milestone

列出变更文件

gh pr diff <PR_NUMBER> --name-only

检查 CI 状态

gh pr checks <PR_NUMBER>

将 diff 下载到文件以便分析

gh pr diff <PR_NUMBER> > /tmp/pr-<PR_NUMBER>.diff

GitLab (glab CLI)

bash
# 查看 MR diff
glab mr diff <MR_IID>

以 JSON 格式查看 MR 详情

glab mr view <MR_IID> --output json

列出变更文件

glab mr diff <MR_IID> --name-only

下载 diff

glab mr diff <MR_IID> > /tmp/mr-<MR_IID>.diff

---

工作流

第一步 — 获取上下文

bash
PR=123
gh pr view $PR --json title,body,labels,milestone,assignees | jq .
gh pr diff $PR --name-only
gh pr diff $PR > /tmp/pr-$PR.diff

第二步 — 影响范围分析

针对每个变更文件，识别：

1. 直接依赖项 — 谁导入了这个文件？

bash
# 查找所有导入了变更模块的文件
grep -r "from ['\"].*changed-module['\"]" src/ --include="*.ts" -l
grep -r "require(['\"].*changed-module" src/ --include="*.js" -l


Python

grep -r "from changed_module import\|import changed_module" . --include="*.py" -l

2. 服务边界 — 此变更是否跨越了服务？

bash
# 检查变更文件是否跨越多个服务（适用于 monorepo）
gh pr diff $PR --name-only | cut -d/ -f1-2 | sort -u

3. 共享契约 — 类型 (types)、接口 (interfaces)、Schema、模型 (models)

bash
gh pr diff $PR --name-only | grep -E "types/|interfaces/|schemas/|models/"

影响范围严重程度：

  • CRITICAL (紧急) — 共享库、数据库模型、鉴权中间件、API 契约

  • HIGH (高) — 被 3 个以上其他服务使用的服务、共享配置、环境变量

  • MEDIUM (中) — 单个服务内部变更、工具函数

  • LOW (低) — UI 组件、测试文件、文档

第三步 — 安全扫描

bash
DIFF=/tmp/pr-$PR.diff

SQL 注入 — 原始查询字符串插值

grep -n "query\|execute\|raw(" $DIFF | grep -E '\$\{|f"|%s|f
ormat\('

硬编码密钥

grep -nE "(password|secret|api_key|token|private_key)\s*=\s*['\"][^'\"]{8,}" $DIFF

AWS 密钥模式

grep -nE "AKIA[0-9A-Z]{16}" $DIFF

代码中的 JWT 密钥

grep -nE "jwt\.sign\(.*['\"][^'\"]{20,}['\"]" $DIFF

XSS 向量

grep -n "dangerouslySetInnerHTML\|innerHTML\s*=" $DIFF

权限绕过模式

grep -n "bypass\|skip.*auth\|noauth\|TODO.*auth" $DIFF

不安全的哈希算法

grep -nE "md5\(|sha1\(|createHash\(['\"]md5|createHash\(['\"]sha1" $DIFF

eval / exec

grep -nE "\beval\(|\bexec\(|\bsubprocess\.call\(" $DIFF

原型链污染

grep -n "__proto__\|constructor\[" $DIFF

路径遍历风险

grep -nE "path\.join\(.*req\.|readFile\(.*req\." $DIFF
code
### 步骤 4 — 测试覆盖率增量 (Test Coverage Delta)
bash

统计源代码与测试文件的变更数量

CHANGED_SRC=$(gh pr diff $PR --name-only | grep -vE "\.test\.|\.spec\.|__tests__") CHANGED_TESTS=$(gh pr diff $PR --name-only | grep -E "\.test\.|\.spec\.|__tests__")

echo "源代码变更文件数: $(echo "$CHANGED_SRC" | wc -w)"
echo "测试代码变更文件数: $(echo "$CHANGED_TESTS" | wc -w)"

新增逻辑行数 vs 新增测试行数

LOGIC_LINES=$(grep "^+" /tmp/pr-$PR.diff | grep -v "^+++" | wc -l) echo "新增行数: $LOGIC_LINES"

在本地运行覆盖率检查

npm test -- --coverage --changedSince=main 2>/dev/null | tail -20 pytest --cov --cov-report=term-missing 2>/dev/null | tail -20
code
覆盖率增量规则：

  • 新增函数但无测试 $\rightarrow$ 标记


  • 删除测试但未删除对应代码 $\rightarrow$ 标记


  • 覆盖率下降 >5% $\rightarrow$ 阻止合并


  • 鉴权/支付路径 $\rightarrow$ 要求 100% 覆盖率


步骤 5 — 破坏性变更检测 (Breaking Change Detection)


#### API 契约变更
bash

OpenAPI/Swagger 规范变更


grep -n "openapi\|swagger" /tmp/pr-$PR.diff | head -20

REST 路由删除或重命名

grep "^-" /tmp/pr-$PR.diff | grep -E "router\.(get|post|put|delete|patch)\("

GraphQL Schema 删除

grep "^-" /tmp/pr-$PR.diff | grep -E "^-\s*(type |field |Query |Mutation )"

TypeScript 接口删除

grep "^-" /tmp/pr-$PR.diff | grep -E "^-\s*(export\s+)?(interface|type) "
code
#### 数据库 Schema 变更
bash

新增迁移文件

gh pr diff $PR --name-only | grep -E "migrations?/|alembic/|knex/"

破坏性操作

grep -E "DROP TABLE|DROP COLUMN|ALTER.*NOT NULL|TRUNCATE" /tmp/pr-$PR.diff

索引删除（存在性能回退风险）

grep "DROP INDEX\|remove_index" /tmp/pr-$PR.diff
code
#### 配置 / 环境变量变更
bash

代码中引用了新的环境变量（生产环境可能缺失）

grep "^+" /tmp/pr-$PR.diff | grep -oE "process\.env\.[A-Z_]+" | sort -u

删除了环境变量（可能导致运行中的实例崩溃）

grep "^-" /tmp/pr-$PR.diff | grep -oE "process\.env\.[A-Z_]+" | sort -u
code
### 步骤 6 — 性能影响分析
bash

N+1 查询模式（循环内调用数据库）

grep -n "\.find\|\.findOne\|\.query\|db\." /tmp/pr-$PR.diff | grep "^+" | head -20

随后检查上下文是否存在 forEach/map/for 循环

引入重量级新依赖

grep "^+" /tmp/pr-$PR.diff | grep -E '"[a-z@].*":\s*"[0-9^~]' | head -20

无界循环

grep -n "while (true\|while(true" /tmp/pr-$PR.diff | grep "^+"

缺失 await（导致 Promise 意外顺序执行）

grep -n "await.*await" /tmp/pr-$PR.diff | grep "^+" | head -10

大内存分配

grep -n "new Array([0-9]\{4,\}\|Buffer\.alloc" /tmp/pr-$PR.diff | grep "^+"
code
---

Ticket 关联验证
bash

从 PR 正文中提取 Ticket 引用

gh pr view $PR --json body | jq -r '.body' | \ grep -oE "(PROJ-[0-9]+|[A-Z]+-[0-9]+|https://linear\.app/[^)
code
\"]+)" | sort -u

验证 Jira 工单是否存在（要求环境变量中已设置 JIRA_API_TOKEN）。


凭据通过从标准输入 (stdin) 读取的配置 (-K -) 传递给 curl，这样 token


就不会出现在 argv 中 —— ps aux 或 /proc/*/cmdline 无法看到它，且


秘密信息不会留在 shell 历史记录中。切勿在命令行中直接粘贴原始 token。

TICKET="PROJ-123"
: "${JIRA_API_TOKEN:?JIRA_API_TOKEN must be set}"
curl -s -K - "https://your-org.atlassian.net/rest/api/3/issue/$TICKET" <<EOF | \
  jq '{key, summary: .fields.summary, status: .fields.status.name}'
user = "[email protected]:$JIRA_API_TOKEN"
EOF

Linear 工单 —— 采用相同模式：Authorization 请求头通过


标准输入配置传递，而非使用 -H 标志，以确保密钥不出现在进程列表中。

LINEAR_ID="abc-123"
: "${LINEAR_API_KEY:?LINEAR_API_KEY must be set}"
curl -s -K - -H "Content-Type: application/json" \
  --data "{\"query\": \"{ issue(id: \\\"$LINEAR_ID\\\") { title state { name } } }\"}" \
  https://api.linear.app/graphql <<EOF | jq .
header = "Authorization: $LINEAR_API_KEY"
EOF

> 安全提示： 对于频繁使用 Jira 的场景，建议使用 ~/.netrc 条目
> (machine your-org.atlassian.net login [email protected] password <token>,
> chmod 600 ~/.netrc) 并调用 curl -s --netrc … —— 这样命令中完全不包含任何秘密信息。

---

完整评审清单 (30+ 项)

markdown
## 代码评审清单

范围与上下文


  • [ ] PR 标题准确描述了变更内容


  • [ ] PR 描述解释了“为什么”而不仅仅是“做了什么”


  • [ ] 关联的 Jira/Linear 工单存在且与范围匹配


  • [ ] 没有无关的变更（避免范围蔓延）


  • [ ] 破坏性变更已在 PR 正文中记录


影响范围 (Blast Radius)


  • [ ] 已识别所有导入了变更模块的文件


  • [ ] 已检查跨服务依赖


  • [ ] 已评审共享类型/接口/Schema 是否会导致崩溃


  • [ ] 新的环境变量已在 .env.example 中记录


  • [ ] 数据库迁移是可逆的（具有 down() / rollback）


安全性


  • [ ] 没有硬编码的密钥或 API Key


  • [ ] SQL 查询使用参数化输入（无字符串拼接）


  • [ ] 用户输入在执行前经过验证/清洗


  • [ ] 所有新端点均有身份验证/授权检查


  • [ ] 无 XSS 风险（如 innerHTML, dangerouslySetInnerHTML）


  • [ ] 新依赖项已检查是否存在已知 CVE 漏洞


  • [ ] 日志中无敏感数据（PII、token、密码）


  • [ ] 文件上传经过验证（类型、大小、content-type）


  • [ ] 新端点的 CORS 配置正确


测试


  • [ ] 新的公共函数有单元测试


  • [ ] 覆盖了边界情况（空值、null、最大值）


  • [ ] 测试了错误路径（而非仅测试正常路径）


  • [ ] API 端点变更已通过集成测试


  • [ ] 删除测试均有明确理由


  • [ ] 测试名称清晰描述了验证内容


破坏性变更


  • [ ] 删除 API 端点前已发出弃用通知


  • [ ] 未在现有 API 响应中添加必填字段


  • [ ] 删除数据库列前已有两阶段迁移计划


  • [ ] 未删除生产环境中可能已设置的环境变量


  • [ ] 对外部 API 调用者保持向后兼容


性能


  • [ ] 未引入 N+1 查询模式


  • [ ] 为新的查询模式添加了数据库索引


  • [ ] 针对潜在大数据集无无限制循环


  • [ ] 引入重量级新依赖时有充分理由


  • [ ] 异步操作已正确 await


  • [ ] 对高开销的重复操作考虑了缓存


代码质量


  • [ ] 无死代码或未使用的导入


  • [ ] 具备错误处理机制（无空的 catch 块）


  • [ ] 与现有模式和约定保持一致
  • [ ] 复杂逻辑附有解释性注释
  • [ ] 没有未解决的 TODO（或已在 Ticket 中跟踪）
code
---

输出格式


评审评论的结构如下：

PR Review: [PR 标题] (#编号)

影响范围 (Blast Radius): 高 — 修改了被 5 个服务使用的 lib/auth
安全性: 1 个发现 (中等严重程度)
测试: 覆盖率变化 +2%
破坏性变更: 未检测到

--- 必须修复 (阻塞) ---

1. src/db/users.ts:42 存在 SQL 注入风险
WHERE 子句中使用了原始字符串插值。
修复方案: db.query("SELECT * WHERE id = $1", [userId])

--- 建议修复 (非阻塞) ---

2. POST /api/admin/reset 缺少权限检查
在执行破坏性操作前未进行角色验证。

--- 优化建议 ---

3. src/services/reports.ts:88 存在 N+1 查询问题
在 results.map() 中调用了 findUser() — 请使用 findManyUsers(ids) 进行批量查询

--- 表现良好 ---

  • 新认证流程的测试覆盖率非常充分

  • DB 迁移脚本包含正确的 down() 回滚方法

  • 错误处理与代码库其余部分保持一致

```

---

常见陷阱

  • 过度关注风格而非实质 — 让 Linter 处理风格问题；重点关注逻辑、安全性和正确性
  • 忽略影响范围 — 共享工具类中 5 行代码的修改可能会导致 20 个服务崩溃
  • 批准未经测试的理想路径 (Happy Paths) — 务必验证错误路径是否已覆盖
  • 忽视迁移风险 — 添加 NOT NULL 约束需要默认值或分两步迁移
  • 间接泄露密钥 — 密钥可能出现在错误消息或日志中，而不仅仅是硬编码
  • 草率审核大型 PR — 如果 PR 过大无法仔细评审，请要求将其拆分

---

最佳实践

1. 在查看代码前先阅读关联的 Ticket — 了解上下文可避免误判
2. 评审前检查 CI 状态 — 不要评审构建失败的代码
3. 影响范围和安全性优先于代码风格
4. 对于非简单的认证或性能变更，在本地进行复现
5. 为每条评论清晰标注标签："nit:" (琐碎), "must:" (必须), "question:" (疑问), "suggestion:" (建议)
6. 在一轮评审中集中提交所有评论 — 避免零散地发送反馈
7. 认可优秀模式，而不仅仅是指出问题 — 具体的赞赏有助于提升团队文化