code-review-workflow

代码审查工作流

核心原则

1. MCP 优先分析 — 在阅读源文件之前，先使用 Roslyn MCP 工具。detect_antipatterns 能发现比手动扫描更多的问题，get_diagnostics 能找出编译器已知的问题，而 find_references 能揭示影响范围。只有在工具无法提供所需上下文时，才阅读文件。
2. 结构化输出 — 每次审查都遵循相同的格式：摘要 → 严重问题 → 警告 → 建议 → 架构合规性 → 测试覆盖率 → 优点。统一的结构能让审查结果更具可操作性，也便于快速浏览。
3. 按严重程度分类问题 — 将每个问题归类为严重问题（合并前必须修复）、警告（应修复，否则会产生技术债务）或建议（锦上添花）。切勿混淆严重程度：把外观问题和安全漏洞放在一起，会掩盖重要问题。
4. 提供可操作的建议 — 每个问题都应说明：问题是什么、为何重要，以及如何修复。“这很糟糕”不是审查意见。“这会导致 N+1 查询，因为 X。可以通过添加 .Include() 或使用投影来修复”才是。
5. 肯定优秀工作 — 始终包含“优点”部分。肯定优秀模式与指出不良模式同样重要。

模式

完整 PR 审查流程

适用于非简单 PR（修改 3 个或更多文件、新功能、重构）。按顺序执行以下步骤：

步骤 1：了解变更范围 通过 git diff 或用户输入获取变更文件。分类如下：

• 新文件（功能、测试、配置）
• 已修改文件（涉及哪些层？领域、应用、基础设施、API？）
• 已删除文件（是否有其他内容依赖这些文件？）

步骤 2：自动化分析 对变更文件运行 MCP 工具：

→ detect_antipatterns (file: each changed .cs file)
  Catch: async void, sync-over-async, DateTime.Now, new HttpClient(), broad catch, etc.

→ get_diagnostics (scope: file, path: each changed file)
  Catch: new compiler warnings, nullability issues, unused variables

→ get_public_api (typeName: each modified type)
  Check: API surface changes — new public members, removed members, signature changes

步骤 3：评估影响范围 对于每个变更的公共 API：

→ find_references (symbolName: changedMethod)
  Count callers. High count = high risk. Flag breaking changes.

步骤 4：架构合规性

→ get_project_graph
  Verify: dependency direction is correct (Domain → nothing, Infra → Domain, Api → Application)
  Flag: circular references, wrong-direction dependencies

步骤 5：检查测试覆盖率

→ get_test_coverage_map (projectFilter: changed project)
  Check: do test files exist for every changed type?
  Flag: new types without tests, modified logic without test updates

步骤 6：手动审查 阅读变更文件，检查工具无法发现的问题：

• 业务逻辑是否正确
• 命名是否清晰且一致
• 错误处理是否完整
• 并发安全性
• 安全性：输入验证、授权检查、数据暴露

步骤 7：生成审查结果

## Review Summary
[1-2 sentence overall assessment: scope, risk level, recommendation]

## Critical (must fix)
- **[File:Line] [Title]** — [What's wrong]. [Why it matters]. [How to fix].
- ...

## Warnings (should fix)
- **[File:Line] [Title]** — [What's wrong]. [Impact if not fixed]. [Suggested fix].
- ...

## Suggestions (nice to have)
- **[File:Line] [Title]** — [Current approach]. [Better alternative]. [Why].
- ...

## Architecture Compliance
[Dependency direction check results. Layer violation findings. Module boundary enforcement.]

## Test Coverage
[Which changed types have tests. Which are missing. Specific test scenarios to add.]

## What's Good
- [Positive finding 1 — reinforce good patterns]
- [Positive finding 2]
- ...

快速审查

适用于小型变更（1-2 个文件、错误修复、配置变更）。采用轻量级流程，跳过影响范围和架构检查。

步骤：

1. 对变更文件运行 detect_antipatterns
2. 对变更文件运行 get_diagnostics
3. 阅读变更代码并检查其正确性
4. 生成精简的审查结果（摘要 + 问题 + 优点）

## Quick Review
[1 sentence assessment]

### Issues
- [Finding with severity tag: 🔴 Critical / 🟡 Warning / 🔵 Suggestion]

### What's Good
- [Positive note]

架构合规性检查

用于独立检查架构层面的问题。适用于审查项目结构变更、新增项目或模块边界调整。

步骤：

1. 运行 get_project_graph — 可视化完整的依赖树
2. 根据架构验证依赖规则：

架构	规则	违规示例
VSA	功能之间不能相互引用	功能 A 从功能 B 导入内容
Clean Architecture	Domain 不引用任何项目	Domain 引用 Infrastructure
DDD	聚合之间不能相互引用	Order 聚合导入 Product 聚合
Modular Monolith	模块只能通过集成事件通信	模块 A 直接引用模块 B 的 DbContext

3. 对模块/层边界类型运行 find_references，验证封装性：

→ find_references(symbolName: "OrdersDbContext")
  Should only be referenced within the Orders module.
  External references = module boundary violation.

4. 运行 detect_circular_dependencies 查找循环依赖：

→ detect_circular_dependencies(scope: projects)
  Flag any project-level cycles.

→ detect_circular_dependencies(scope: types, projectFilter: "MyApp.Application")
  Flag type-level cycles within the application layer.

反模式

不使用 MCP 工具进行审查

# BAD — Reading every file manually, missing patterns across the codebase
"Let me read OrderService.cs... looks fine to me."
# Missed: 3 DateTime.Now usages, 1 async void, 2 compiler warnings

# GOOD — MCP-first, then targeted file reads
→ detect_antipatterns: Found 3 DateTime.Now (AP004), 1 async void (AP001)
→ get_diagnostics: 2 CS8600 warnings in OrderService.cs
"I found 6 issues via static analysis. Let me read the files for business logic review..."

含糊的反馈

# BAD
"The code could be better."
"This doesn't look right."
"Consider refactoring this."

# GOOD
"OrderService.cs:47 — `DateTime.Now` should be `TimeProvider.GetUtcNow()`.
DateTime.Now is untestable and uses local timezone. Inject TimeProvider
via primary constructor and call GetUtcNow()."

缺少安全检查

# BAD — Only checking code style and patterns
"Code looks clean, approved!"
# Missed: SQL injection in raw query, missing authorization attribute, exposed PII in logs

# GOOD — Security is a review dimension
"## Critical
- **OrderController.cs:23** Missing `[Authorize]` — endpoint exposes order data without auth
- **SearchService.cs:45** SQL injection — user input concatenated into raw SQL. Use parameterized query.
## Suggestions
- **LoggingMiddleware.cs:12** PII exposure — email logged at Information level. Mask or use Debug level."

纠结于风格问题，却忽略实质问题

# BAD — 10 comments about naming, 0 about the race condition
"Rename `svc` to `service`. Use `var` instead of explicit type. Add XML docs."

# GOOD — Prioritize by impact
"## Critical
- Race condition in OrderService.ProcessAsync — concurrent calls can double-charge
## Suggestions
- Consider renaming `svc` to `service` for clarity"

Decision Guide

Scenario	Review Type	MCP Tools
Feature PR (3+ files)	Full PR Review	All tools
Bug fix (1-2 files)	Quick Review	detect_antipatterns, get_diagnostics
Config/infra changes	Quick Review + Manual	get_project_graph
New project/module added	Architecture Compliance	get_project_graph, detect_circular_dependencies
Refactor PR	Full PR Review + Architecture	All tools + find_references (blast radius)
Security-sensitive change	Full PR Review → escalate to security-auditor	detect_antipatterns + manual security review
Test-only changes	Quick Review	get_diagnostics only
Performance-critical path	Full PR Review → escalate to performance-analyst	get_diagnostics + manual review