pr-review-expert

PR 审查专家

层级: POWERFUL 分类: Engineering 领域: 代码审查 / 质量保证

---

概述

为 GitHub PR 和 GitLab MR 提供结构化、系统化的代码审查。本技能不止于风格挑剔,还会执行影响半径分析、安全扫描、破坏性变更检测以及测试覆盖率增量计算。最终生成包含 30 多项检查清单和优先级排序发现的、可供审查者直接使用的报告。

---

核心能力

• 影响半径分析 — 追踪哪些文件、服务和下游使用方可能受到破坏
• 安全扫描 — SQL 注入、XSS、认证绕过、密钥泄露、依赖漏洞
• 测试覆盖率增量 — 新代码与新测试的比例
• 破坏性变更检测 — API 契约、数据库 schema 迁移、配置项
• 工单关联 — 验证 Jira/Linear 工单存在且与变更范围一致
• 性能影响 — N+1 查询、打包体积回退、内存分配

---

适用场景

• 合并任何涉及共享库、API 或数据库 schema 的 PR/MR 之前
• PR 较大(变更超过 200 行)且需要结构化审查时
• 为新贡献者的 PR 提供详细反馈时
• 安全敏感的代码路径(认证、支付、PII 处理)
• 发生事故之后,主动审查类似的 PR

---

获取 Diff

GitHub (gh CLI)

# View diff in terminal
gh pr diff <PR_NUMBER>

# Get PR metadata (title, body, labels, linked issues)
gh pr view <PR_NUMBER> --json title,body,labels,assignees,milestone

# List files changed
gh pr diff <PR_NUMBER> --name-only

# Check CI status
gh pr checks <PR_NUMBER>

# Download diff to file for analysis
gh pr diff <PR_NUMBER> > /tmp/pr-<PR_NUMBER>.diff

GitLab (glab CLI)

# View MR diff
glab mr diff <MR_IID>

# MR details as JSON
glab mr view <MR_IID> --output json

# List changed files
glab mr diff <MR_IID> --name-only

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

---

工作流

第 1 步 — 收集上下文

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

第 2 步 — 影响半径分析

对于每个变更文件,确定以下内容:

1. 直接依赖方 — 哪些模块导入了该文件?

# Find all files importing a changed module
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. 服务边界 — 此变更是否跨越了服务边界?

# Check if changed files span multiple services (monorepo)
gh pr diff $PR --name-only | cut -d/ -f1-2 | sort -u

3. 共享契约 — 类型、接口、schema

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

影响半径严重程度:

• CRITICAL — 共享库、数据库模型、认证中间件、API 契约
• HIGH — 被超过 3 个其他服务使用的服务、共享配置、环境变量
• MEDIUM — 单个服务的内部变更、工具函数
• LOW — UI 组件、测试文件、文档

第 3 步 — 安全扫描

DIFF=/tmp/pr-$PR.diff

# SQL Injection — raw query string interpolation
grep -n "query\|execute\|raw(" $DIFF | grep -E '\$\{|f"|%s|format\('

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

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

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

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

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

# Insecure hash algorithms
grep -nE "md5\(|sha1\(|createHash\(['\"]md5|createHash\(['\"]sha1" $DIFF

# eval / exec
grep -nE "\beval\(|\bexec\(|\bsubprocess\.call\(" $DIFF

# Prototype pollution
grep -n "__proto__\|constructor\[" $DIFF

# Path traversal risk
grep -nE "path\.join\(.*req\.|readFile\(.*req\." $DIFF

第 4 步 — 测试覆盖率增量

# Count source vs test files changed
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 "Source files changed: $(echo "$CHANGED_SRC" | wc -w)"
echo "Test files changed:   $(echo "$CHANGED_TESTS" | wc -w)"

# Lines of new logic vs new test lines
LOGIC_LINES=$(grep "^+" /tmp/pr-$PR.diff | grep -v "^+++" | wc -l)
echo "New lines added: $LOGIC_LINES"

# Run coverage locally
npm test -- --coverage --changedSince=main 2>/dev/null | tail -20
pytest --cov --cov-report=term-missing 2>/dev/null | tail -20

覆盖率增量规则:

• 新函数没有测试 → 标记
• 删除了测试但未删除对应代码 → 标记
• 覆盖率下降超过 5% → 阻止合并
• 认证/支付路径 → 要求 100% 覆盖率

第 5 步 — 破坏性变更检测

API 契约变更

# OpenAPI/Swagger spec changes
grep -n "openapi\|swagger" /tmp/pr-$PR.diff | head -20

# REST route removals or renames
grep "^-" /tmp/pr-$PR.diff | grep -E "router\.(get|post|put|delete|patch)\("

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

# TypeScript interface removals
grep "^-" /tmp/pr-$PR.diff | grep -E "^-\s*(export\s+)?(interface|type) "

数据库 Schema 变更

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

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

# Index removals (perf regression risk)
grep "DROP INDEX\|remove_index" /tmp/pr-$PR.diff

配置 / 环境变量变更

# New env vars referenced in code (might be missing in prod)
grep "^+" /tmp/pr-$PR.diff | grep -oE "process\.env\.[A-Z_]+" | sort -u

# Removed env vars (could break running instances)
grep "^-" /tmp/pr-$PR.diff | grep -oE "process\.env\.[A-Z_]+" | sort -u

第 6 步 — 性能影响

# N+1 query patterns (DB calls inside loops)
grep -n "\.find\|\.findOne\|\.query\|db\." /tmp/pr-$PR.diff | grep "^+" | head -20
# Then check surrounding context for forEach/map/for loops

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

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

# Missing await (accidentally sequential promises)
grep -n "await.*await" /tmp/pr-$PR.diff | grep "^+" | head -10

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

---

工单关联验证

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

# Verify Jira ticket exists (requires JIRA_API_TOKEN to be SET in the environment).
# Credentials are fed to curl via a config read from stdin (-K -) so the token
# never appears in argv — `ps aux` / /proc/*/cmdline can't see it, and nothing
# secret lands in shell history. Never paste the raw token on the command line.
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 ticket — same pattern: the Authorization header goes through the
# stdin config, not a -H flag, to keep the key out of the process list.
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 多项)

## Code Review Checklist

### Scope & Context
- [ ] PR title accurately describes the change
- [ ] PR description explains WHY, not just WHAT
- [ ] Linked Jira/Linear ticket exists and matches scope
- [ ] No unrelated changes (scope creep)
- [ ] Breaking changes documented in PR body

### Blast Radius
- [ ] Identified all files importing changed modules
- [ ] Cross-service dependencies checked
- [ ] Shared types/interfaces/schemas reviewed for breakage
- [ ] New env vars documented in .env.example
- [ ] DB migrations are reversible (have down() / rollback)

### Security
- [ ] No hardcoded secrets or API keys
- [ ] SQL queries use parameterized inputs (no string interpolation)
- [ ] User inputs validated/sanitized before use
- [ ] Auth/authorization checks on all new endpoints
- [ ] No XSS vectors (innerHTML, dangerouslySetInnerHTML)
- [ ] New dependencies checked for known CVEs
- [ ] No sensitive data in logs (PII, tokens, passwords)
- [ ] File uploads validated (type, size, content-type)
- [ ] CORS configured correctly for new endpoints

### Testing
- [ ] New public functions have unit tests
- [ ] Edge cases covered (empty, null, max values)
- [ ] Error paths tested (not just happy path)
- [ ] Integration tests for API endpoint changes
- [ ] No tests deleted without clear reason
- [ ] Test names clearly describe what they verify

### Breaking Changes
- [ ] No API endpoints removed without deprecation notice
- [ ] No required fields added to existing API responses
- [ ] No DB columns removed without two-phase migration plan
- [ ] No env vars removed that may be set in production
- [ ] Backward-compatible for external API consumers

### Performance
- [ ] No N+1 query patterns introduced
- [ ] DB indexes added for new query patterns
- [ ] No unbounded loops on potentially large datasets
- [ ] No heavy new dependencies without justification
- [ ] Async operations correctly awaited
- [ ] Caching considered for expensive repeated operations

### Code Quality
- [ ] No dead code or unused imports
- [ ] Error handling present (no bare empty catch blocks)
- [ ] Consistent with existing patterns and conventions
- [ ] Complex logic has explanatory comments
- [ ] No unresolved TODOs (or tracked in ticket)

---

输出格式

将审查评论按如下结构组织:

## PR Review: [PR Title] (#NUMBER)

Blast Radius: HIGH — changes lib/auth used by 5 services
Security: 1 finding (medium severity)
Tests: Coverage delta +2%
Breaking Changes: None detected

--- MUST FIX (Blocking) ---

1. SQL Injection risk in src/db/users.ts:42
   Raw string interpolation in WHERE clause.
   Fix: db.query("SELECT * WHERE id = $1", [userId])

--- SHOULD FIX (Non-blocking) ---

2. Missing auth check on POST /api/admin/reset
   No role verification before destructive operation.

--- SUGGESTIONS ---

3. N+1 pattern in src/services/reports.ts:88
   findUser() called inside results.map() — batch with findManyUsers(ids)

--- LOOKS GOOD ---
- Test coverage for new auth flow is thorough
- DB migration has proper down() rollback method
- Error handling consistent with rest of codebase

---

常见误区

• 重风格轻实质 — 风格问题交给 linter 处理;应专注于逻辑、安全和正确性
• 忽略影响半径 — 共享工具函数中一个 5 行的改动可能破坏 20 个服务
• 批准未经测试的正常路径 — 务必验证错误路径是否有覆盖
• 忽视迁移风险 — 新增 NOT NULL 字段需要设置默认值或采用两阶段迁移
• 间接的密钥泄露 — 密钥出现在错误信息或日志中,而不仅仅是硬编码的值里
• 跳过大型 PR — 如果 PR 过大而无法充分审查,应要求将其拆分

---

最佳实践

1. 在查看代码之前先阅读关联的工单 — 上下文可以避免误报
2. 审查前检查 CI 状态 — 不要审查无法构建的代码
3. 审查时优先关注影响半径和安全,而非风格
4. 对于非平凡的认证或性能变更,在本地复现
5. 为每条评论清晰标注:"nit:"、"must:"、"question:"、"suggestion:"
6. 将所有评论集中在一轮审查中提出 — 不要零散地反馈
7. 既要指出问题,也要肯定良好的实践 — 具体的表扬有助于营造良好的团队文化