sdd-review

SDD: コードレビュー

あなたはプリンシパルエンジニアとして、徹底的なコードレビューを行います。 レビューは誠実で、具体的で、実行可能でなければなりません — 一般的な内容ではいけません。 すべての指摘は、正確なファイルと行範囲を参照する必要があります。

入力

入力	必須	説明	例
review_path	任意	レビュー対象のファイル、パッケージ、またはモジュールのパス。デフォルトは git diff の全範囲です。	src/auth/

手順

ステップ 0: 入力の検証 (必ず最初に行います)

• review_path が指定されている場合 → レビューの範囲をそのパスだけに限定します。ステップ 1 に進みます。
• review_path がない場合 → 範囲を自動的に決定します:
  1. git diff main...HEAD --name-only を実行して、このブランチで変更されたファイルを特定します。
  2. フィーチャーブランチ上でない場合は、パスを指定するか、コードベース全体のレビューを希望するかをユーザーに確認します。 範囲が確定したらステップ 1 に進みます。

---

事前条件

開始する前に以下を読んでください:

• docs/project.md — 技術スタック、アーキテクチャ、規約
• feature.md — 受け入れ基準と機能要件 (存在する場合)
• plan.md — 意図された実装アプローチ (存在する場合)

範囲

ステップ 0 で確定したパスまたはファイルセットをレビューします。変更・追加されたファイルに焦点を当て、無関係な既存コードには言及しますが詳細にはレビューしません。

---

レビューの観点

以下の各観点を順番に確認します。各指摘について、該当する重大度テーブル (「出力形式」を参照) に 1 行追加します:

| [ ] | `path/to/File.java:line` | <category> | <one sentence: what is wrong and why it matters> | <one sentence: concrete fix> |

• 最初の列 [ ] はステータスチェックボックスです — 指摘が解決されたら [x] に変更します。
• Problem と Suggestion は 1 文に収めてください。テーブルセル内にコードブロックを入れないでください。

重大度レベル:

• 🔴 CRITICAL — マージ前に必ず修正 (セキュリティホール、データ損失のリスク、壊れた AC)
• 🟠 MAJOR — マージ前に修正すべき (重大なバグ、深刻な設計上の欠陥)
• 🟡 MINOR — 早めに修正するがブロッカーではない (コードスメル、軽微な非効率)
• 🔵 INFO — 提案またはベストプラクティス (スタイル、任意の改善)

---

観点 1: 受け入れ基準の検証

feature.md が存在する場合、すべての AC を確認します:

• それを直接カバーするテストがあることを確認します
• 実装が実際にそれを満たしていることを確認します (テストが存在するだけでは不十分です)
• テストカバレッジがない AC は 🔴 CRITICAL として報告します
• 完全にカバーされ満たされている各 AC について、feature.md 内で完了としてマークします — その AC の行の - [ ] を - [x] に変更します

---

観点 2: 言語とフレームワークのベストプラクティス

docs/project.md で宣言された技術スタックの規約と慣用的なパターンに照らしてレビューします。 プロジェクトに存在する lint ルール、スタイルガイド、フォーマッター設定があれば参照します。

言語

• 使用している言語にとって慣用的なコードである — 現代的な言語機能が適切に使われている
• この言語に特有のアンチパターンがない (リソースリーク、安全でない型変換、無視されたエラーなど)
• エラーハンドリングがプロジェクトで宣言された規約に従っている (例外、エラー戻り値、Result 型など)
• 本番コードにデバッグ出力が残っていない (print、console.log など) — 構造化ログのみ
• 言語が対応している場合は不変性または値セマンティクスを優先している

フレームワーク

• フレームワークが推奨するレイヤーの責務に従っている — プレゼンテーション/コントローラーレイヤーにビジネスロジックがない
• 設定がフレームワークの規約に従って一元化されている — インラインの設定値が散在していない
• 依存性注入またはサービスのワイヤリングがフレームワークの標準的な仕組みを使用している
• HTTP ステータスコードが結果に対して意味的に正しい
• エラー/例外ハンドリングが一元化されている (ミドルウェア、ハンドラー、フィルター) — エンドポイントごとに重複していない
• テストは利用可能な最も狭いテストスコープを使用している — 十分な場合はフルスタックテストよりもユニットテストやスライステストを優先する

データアクセス

• 潜在的に大規模なデータセットに対する無制限のクエリは行わない — 適切な箇所ではページネーションを適用する
• N+1 クエリのリスクを特定し対処する（eager loading、バッチ処理、または明示的な join）
• クエリはフレームワークの安全なパラメーター化機構を使用する — クエリ内での文字列連結は行わない
• レコードが存在しない場合は、使用前に明示的に処理する（null チェック、空のオプショナルガードなど）

---

ディメンション 3: セキュリティ

• インジェクション: クエリ内での文字列連結は行わない（SQL、NoSQL など） — パラメーター化クエリのみを使用する
• 認証と認可: 機密性の高いエンドポイントは保護されている。サーバー側の検証なしに、クライアントから提供されたデータのみに基づいてセキュリティ上の判断を行わない
• 入力検証: すべてのリクエストボディとパラメーターは使用前に検証する
• 機密データの露出: パスワード、トークン、PII、シークレットをログに記録したり、API レスポンスで返したりしない
• マスアサインメント: 明示的なフィールドフィルタリングなしに、リクエスト入力を永続化モデルに直接バインドしない
• 依存関係のリスク: docs/project.md の承認済みスタックに含まれない新しい依存関係があれば指摘する
• CORS / CSRF: 新しいエンドポイントを追加する場合、CORS 設定が過度に許容的でないことを確認する
• エラーメッセージ: スタックトレースや内部の詳細がエラーレスポンスで漏洩しないようにする

---

ディメンション 4: コードの重複

• 変更されたファイルをスキャンし、コードベース内の既存のユーティリティ、サービス、ヘルパーを重複するロジックがないか確認する
• 新しいテストクラス間やサービスのメソッド間でのコピー＆ペーストを指摘する
• ポリモーフィズムやストラテジーパターンにすべき、繰り返される if/else や switch ブロックを特定する
• 複数の箇所に出現し、定数や設定にすべきハードコードされた値を指摘する

---

ディメンション 5: 設計とアーキテクチャ

• コードは docs/project.md のレイヤリングを尊重している（例: ドメインロジックがコントローラーに漏れていない、 Hexagonal/Clean Architecture においてデータアクセスやインフラストラクチャのコードがドメイン層にない）
• クラスは単一責任の原則に従っている — 多くのことを担いすぎているクラスを指摘する
• 状態を保持する不適切な static メソッドがない
• インターフェースと抽象化を適切に使用している — 過剰設計ではなく、かつ意味のある抽象化を省略してもいない
• パッケージ構造が既存の規約と一致している

---

ディメンション 6: パフォーマンス

• リアクティブ／非同期パイプライン内に同期的なブロッキング呼び出しがない（該当する場合）
• ループ内での繰り返しデータベース呼び出しがない — 可能な箇所ではバッチ処理する
• 高コストな操作（例: 外部 API 呼び出し、ファイル I/O）が、キャッシュの検討なしにホットパスに存在しない
• クエリパターンから示唆されるインデックス — 特定可能であれば、インデックスのないカラムに対するクエリを指摘する

---

ディメンション 7: テストの品質

• テストは Arrange-Act-Assert 構造に従っている
• テスト名がシナリオを明確に説明している（should_returnError_when_emailAlreadyExists）
• テスト内にロジック（if、for ループ）がない — 各テストは単一の明確なシナリオである
• モックはアーキテクチャの境界でのみ使用する — 同じモジュールが所有するクラスをモックしない
• テスト内に任意の sleep がない — 非同期アサーションには適切な非同期／ポーリングユーティリティを使用する
• テストデータは最小限かつ焦点を絞ったものである — テスト対象を曖昧にする肥大化したセットアップがない
• エッジケースをカバーしている: null 入力、空のコレクション、境界値

---

ディメンション 8: 可観測性

• 新しいサービスのメソッドと主要なビジネスイベントに、適切なレベルで適切なログステートメントがある （診断の詳細には DEBUG、ビジネスイベントには INFO、回復可能な問題には WARN、失敗には ERROR）
• ログメッセージに機密データがない
• プロジェクトがメトリクスライブラリを使用している場合、新たな重要な操作が計測されている

---

出力形式

この構造を使用して、プロジェクトルートの review.md にレビューを書き込みます:

# Code Review: <Feature Name or Path>

## Summary
<2-3 sentence overall assessment. Be direct — is this ready to merge, needs minor fixes, or needs significant rework?>

## Findings

### 🔴 Critical

| Done | Location | Category | Problem | Suggestion |
|------|----------|----------|---------|------------|
| [ ] | `src/service/AuthService.java:42` | Null Safety | `user` may be null; NPE at runtime | Add null guard before line 42 |

### 🟠 Major

| Done | Location | Category | Problem | Suggestion |
|------|----------|----------|---------|------------|

### 🟡 Minor

| Done | Location | Category | Problem | Suggestion |
|------|----------|----------|---------|------------|

### 🔵 Info / Suggestions

| Done | Location | Category | Problem | Suggestion |
|------|----------|----------|---------|------------|

## Acceptance Criteria Coverage
| AC         | Test               | Status          |
|------------|--------------------|-----------------|
| AC-01: ... | `FooTest#test_...` | ✅ Covered       |
| AC-02: ... | —                  | ❌ No test found |

## Verdict
- [ ] ✅ Ready to merge
- [ ] 🟡 Merge after minor fixes (no re-review needed)
- [ ] 🟠 Requires fixes and re-review
- [ ] 🔴 Do not merge — significant issues found

ファイルを書き込んだ後、1 行の確認を出力します: review.md written. 次に、ユーザーがファイルを開かなくてもすぐに状況を把握できるよう、Summary と Verdict のセクションをインラインで表示します。

---

レビュー後

ユーザーに尋ねます：

「これらの指摘のうち、今すぐ修正してほしいものはありますか？『critical と major をすべて修正』と言うこともできますし、特定の項目を指定することもできます。」

ユーザーが修正を依頼した場合はそれに対応し、その後、関連するテストを再実行して修正が維持されていることを確認します。 すべての指摘が解決したら、まだ実行していなければ /sdd-archive を実行するようユーザーに促します。