pr-review

PR レビュー

このスキルでは、観点を絞ったプルリクエストレビューを扱います。適切な観点（セキュリティ、パフォーマンス、テスト、アーキテクチャ）を選び、具体的で重大度順に並べた指摘を提示し、明確なマージ判断で締めくくります。

PR レビューのワークフロー

1. 観点を選ぶ — ユーザーのリクエスト（「security」「perf」「tests」「arch」）から重点を判断します。指定がない場合は、どの観点を使うか尋ねるか、最もリスクが高い既定値としてセキュリティを選びます。
2. ファイル全体のコンテキストを確認する — 差分だけでは、変更行のすぐ外側にあるバグを見落とすことがよくあります。差分しかない場合は、確信を持って指摘する前に、周辺のファイルを求めてください。
3. 観点のチェックリストに沿ってレビューする — 以下の該当するチェックリストを優先度順に確認します。ユーザーが全体レビューを求めていない限り、複数の観点を混ぜないでください。
4. 具体的で引用付きの指摘を書く — すべての指摘で、ファイル名と行番号を挙げ、曖昧な印象ではなく具体的な欠陥を示します。
5. 重大度で順位付けする — 指摘を blocker、important、nit に分類します。
6. 不確実性を明示する — 差分だけでは確信を持つのに十分なコンテキストがない場合は、その旨を伝え、推測せずに追加のコードを求めてください。
7. 最後に独立した行で判断を示す — Safe to merge | needs changes | reject（または以下のアーキテクチャ固有の判断）。

出力のルール

すべてのレビュー観点に適用されます。

• 指摘ごとにファイルパスと行番号を記載してください。
• 指摘を重大度順に並べてください：blocker、important、nit。
• 具体的に書いてください。「これは危険そうです」は指摘ではありません。「src/auth.ts:42 — JWT シークレットをリクエストボディから読み取っています。41 行目を参照してください」は指摘です。
• 差分だけでは確信を持つのに十分なコンテキストがない場合は、その旨を明示し、周辺のファイルを求めてください。
• 最後に独立した行で判断を示してください：Safe to merge | needs changes | reject。

観点 1：セキュリティ

以下の優先度順で、PR のセキュリティ上の欠陥をレビューします。

1. 認証／認可 — 新しいエンドポイントや分岐に認証チェックがない、ロールを誤って前提としている、IDOR（安全でない直接オブジェクト参照）。
2. 入力検証 — 信頼できない入力がクエリ、シェルコマンド、ファイルパス、デシリアライズ、または eval に渡されている。
3. インジェクション — SQL、NoSQL、コマンド、プロンプト、テンプレートのインジェクション。
4. シークレット — キーやトークンのハードコード、ログへのシークレット出力、クライアントにバンドルされるコード内のシークレット、リポジトリにコミットされた .env。
5. 出力エンコーディング — エスケープされていないテンプレートによる XSS、ユーザーコンテンツ内の生 HTML、JSONP のような情報漏えい。
6. 暗号／乱数 — トークンに Math.random() を使用している、パスワードや完全性の検証に MD5／SHA1 を使用している、IV がない、独自実装の暗号。
7. データ漏えい — ログ内の PII、過剰な API レスポンス、マスキングの欠如。

あれば望ましい程度の改善は省き、欠陥に絞ってください。

観点 2：パフォーマンス

パフォーマンスのリグレッションをレビューします。

1. N+1 パターン — バッチ処理ではなく、ループ内で項目ごとに DB やネットワークを呼び出している。
2. ホットパスでの割り当て — ループ内で新しいオブジェクト／配列／マップを作成している、呼び出しごとに正規表現を再コンパイルしている。
3. 処理量に上限がない — ページネーションがない、結果セットに制約がない、深さの上限なしに再帰している。
4. 非効率な非同期処理 — Promise.all が適切なのに await を逐次実行している、並行数の制限がない。
5. キャッシュの誤用 — 関連する変数をキャッシュキーに含めていない、不適切または極端な TTL。
6. アルゴリズム計算量 — 隠れた O(n²)（例：.map() 内の .some()）、ループ内のソート。

該当する行を引用し、計算量クラスまたは問題のパターンを示して、修正方法を提案してください。

観点 3：テスト

PR のテストカバレッジを確認します。

1. 新しいコードパスのテスト — 新しい分岐にはそれぞれ、少なくとも 1 つのテストが必要です。
2. エッジケース — 空の入力、null／undefined、境界値、依存先がスローするエラー。
3. アサーションの強度 — 誤った値でも通過するアサーション、スナップショットのみのテスト、正常系しか確認しないテスト。
4. モックの適切な使用 — 実際のインターフェースが変わっても失敗しないモック、実際の動作を隠してしまう過剰なモック。
5. 決定性 — 日時、乱数、ネットワークがスタブ化されておらず、テストが不安定になっている状態。
6. テスト名 — テスト対象の動作を説明していない名前。

テストが存在することと、回帰を検出できるテストがあることは同じではありません。テスト名ではなく、アサーションを確認してください。

観点 4：アーキテクチャ

行単位の懸念から離れ、変更全体の構造を確認します。

1. 境界のずれ — レイヤー間の境界はどこで移動しましたか？ UI が DB に直接アクセスするようになりましたか？ ドメイン型がトランスポート型をインポートするようになりましたか？
2. 時期尚早な抽象化 — 実装が 1 つしかないインターフェース、ファクトリー、設定レイヤー。これらは負債として指摘してください。
3. 結合 — ユーティリティが機能モジュールからインポートするようになっていないか、共有の可変状態が導入されていないか。
4. スケーラビリティ — このコードパスの規模が 10 倍になったら、最初に何が壊れますか？
5. 可逆性 — 1 か月後にこの変更が誤りだと判明した場合、ロールバックはどれほど難しいですか？ 後戻りできない決定は明示的に指摘してください。
6. 命名 — UserManagerImplV2 のように実装に基づいて型／関数に名前を付けるのではなく、UserDirectory のように役割に基づいて命名します。

最後に、次のいずれかで締めくくります：アーキテクチャは健全 | 簡素化が必要 | マージ前に再考が必要。

指摘事項の形式例

src/api/orders.ts:118 (blocker, security)
  Order lookup uses `req.params.orderId` directly in a WHERE clause
  built via string concatenation — SQL injection. Use a parameterized
  query or the existing `db.orders.findById()` helper instead.

src/api/orders.ts:142 (nit, tests)
  `createOrder` has no test for the case where `items` is an empty
  array; current tests only cover the happy path with 1-3 items.

Verdict: needs changes

複数の観点を同時にレビューする

• ユーザーが特定の観点ではなく「フルレビュー」を求めている場合は、4 つのチェックリストを混ぜて一度に確認するのではなく、順番にすべて確認してください。観点ごとに検出できる不具合の種類が異なり、分けておくことで指摘事項と特定の懸念を結び付けやすくなります。
• 各観点をそれぞれ独自の見出しと判定行の下に示してください。4 つの判定を 1 つの要約文にまとめないでください。
• 時間やコンテキストが限られている場合は、まずセキュリティ、次にテスト、パフォーマンス、アーキテクチャの順に優先してください。セキュリティ上の不具合がリリースされることは、アーキテクチャ上の問題よりも明らかに深刻です。

レビューでよくある落とし穴

• コードが実際に何をしているかを読まず、差分の意図（「バリデーションを追加しているようだ」）を根拠に承認する。
• テストスイートが通過したことを正しさの証明とみなす。テストスイートが証明するのは、存在するテストが通過したことだけであり、必要なテストが存在することではありません。
• レビューの範囲を広げすぎる。差分で変更されていない既存の問題を個別に指摘するのは構いませんが、レビュー対象の PR をブロックすべきではありません。
• 判断を裏付けるのに十分な周辺ファイルを確認せずに判定を出す。推測するのではなく、追加のコンテキストを求めてください。