code-review-workflow

コードレビューのワークフロー

基本原則

1. MCP を優先した分析 — ソースファイルを読む前に、Roslyn MCP ツールを使用します。detect_antipatterns は手作業でのスキャンより多くの問題を検出し、get_diagnostics はコンパイラーが把握している問題を見つけ、find_references は影響範囲を明らかにします。ツールでは得られないコンテキストを確認する場合にのみ、ファイルを読みます。
2. 構造化された出力 — すべてのレビューで同じ形式に従います：概要 → 重大な問題 → 警告 → 提案 → アーキテクチャへの準拠 → テストカバレッジ → 良い点。一貫した構成により、レビューを実行可能で把握しやすいものにできます。
3. 重大度に基づく指摘 — すべての指摘を「重大」（マージ前に必ず修正）、「警告」（修正すべきであり、技術的負債を生む）、「提案」（あると望ましい）に分類します。重大度を混在させないでください。セキュリティ上のバグの隣に見た目だけの問題を並べると、重要な指摘が埋もれてしまいます。
4. 実行可能な提案 — すべての指摘には、問題点、重要である理由、修正方法を含めます。「これはよくありません」はレビューコメントではありません。「X が原因で N+1 クエリが発生します。.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 からインポートする
クリーンアーキテクチャ	ドメインにはプロジェクト参照が一切ない	ドメインがインフラストラクチャを参照する
DDD	集約同士は互いを参照しない	Order 集約が Product 集約をインポートする
モジュラーモノリス	モジュール間の通信は統合イベント経由に限る	モジュール 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