コードレビューと品質

概要

品質ゲート付きの多次元コードレビュー。すべての変更はマージ前にレビューされます — 例外なし。レビューは5つの軸でカバーされます：正確性、可読性、アーキテクチャ、セキュリティ、パフォーマンス。

承認の基準： 変更が完璧でなくても、全体的なコード品質を確実に向上させる場合は承認します。完璧なコードは存在しません — 目標は継続的な改善です。自分の書き方と異なるというだけで変更をブロックしないでください。コードベースを改善し、プロジェクトの規約に従っていれば、承認してください。

使用時機

PRまたは変更をマージする前
フィーチャー実装を完了した後
別のエージェントまたはモデルが作成したコードを評価する必要がある場合
既存コードをリファクタリングする場合
バグ修正後（修正と回帰テスト両方をレビュー）

5つの軸のレビュー

すべてのレビューは以下の次元でコードを評価します：

1. 正確性

コードが主張していることを実際に行っているか？

仕様またはタスク要件と一致しているか？
エッジケース（null、空、境界値）が処理されているか？
エラーパス（正常系だけでなく）が処理されているか？
すべてのテストに合格しているか？テストは本当に正しいことをテストしているか？
オフバイワンエラー、競合状態、状態の不一貫性がないか？

2. 可読性とシンプルさ

別のエンジニア（またはエージェント）がこのコードを著者の説明なしに理解できるか？

名前は説明的でプロジェクトの規約と一貫しているか？（コンテキストなしのtemp、data、resultがないか）
制御フローが直感的か（ネストされた三項演算子、深いコールバックを避けているか）？
コードが論理的に組織されているか（関連するコードがグループ化され、モジュール境界が明確か）？
シンプル化すべき「賢い」トリックがないか？
これをもっと少ない行数でできるか？（100行で十分な場合に1000行は失敗）
抽象化は複雑さに見合っているか？（3番目のユースケースまで一般化しない）
コメントが自明でない意図を明確にするのに役立つか？（ただし、自明なコードにはコメントしない）
デッドコード：使われていない変数（_unused）、後方互換性シム、// removedコメント？

3. アーキテクチャ

変更はシステムの設計に適合しているか？

既存パターンに従うか、新しいパターンを導入するか？新しい場合、正当化されているか？
清潔なモジュール境界を維持しているか？
共有すべきコード重複がないか？
依存関係が正しい方向に流れているか（循環依存がないか）？
抽象化レベルは適切か（過度に設計されていない、過度に結合していない）？

4. セキュリティ

詳細なセキュリティガイダンスはsecurity-and-hardeningを参照してください。変更は脆弱性を導入しないか？

ユーザー入力は検証・サニタイズされているか？
シークレットはコード、ログ、バージョン管理から除外されているか？
必要な場所で認証・認可がチェックされているか？
SQLクエリはパラメータ化されているか（文字列連結がないか）？
出力はXSSを防止するようにエンコードされているか？
依存関係は信頼できるソースから取得され、既知の脆弱性がないか？
外部ソース（API、ログ、ユーザーコンテンツ、設定ファイル）からのデータは信頼できないものとして扱われているか？
外部データフローはロジックまたはレンダリングで使用される前にシステム境界で検証されているか？

5. パフォーマンス

詳細なプロファイリングと最適化についてはperformance-optimizationを参照してください。変更はパフォーマンス問題を導入しないか？

N+1クエリパターンがないか？
無限ループや制約のないデータ取得がないか？
非同期にすべき同期操作がないか？
UIコンポーネントの不要な再レンダリングがないか？
リストエンドポイントにペーグネーションがないか？
ホットパスで作成される大きなオブジェクトがないか？

変更のサイズ

小さく、焦点を絞った変更はレビューが簡単で、マージが速く、デプロイがより安全です。以下のサイズを目標にしてください：

~100行変更   → よい。1回のセッションでレビュー可能。
~300行変更   → 単一の論理的変更であれば受け入れ可能。
~1000行変更  → 大きすぎる。分割してください。

「1つの変更」とは何か： 1つのことに対応し、関連するテストを含み、提出後もシステムが機能する、単一の自己完結した修正。フィーチャー全体ではなく、フィーチャーの1部。

変更が大きすぎる場合の分割戦略：

戦略	方法	使用時機
スタック	小さな変更を提出し、それに基づいて次の変更を開始	順序立った依存関係
ファイルグループ別	異なるレビュアーが必要なグループに対して変更を分離	横断的な関心事
水平	共有コード・スタブを最初に作成し、次に消費者	レイヤー化されたアーキテクチャ
垂直	フィーチャーの小さいフルスタックスライスに分割	フィーチャー作業

大きな変更が受け入れられる場合： 完全なファイル削除と自動リファクタリング（レビュアーは意図を確認するだけで、すべての行を確認する必要はない）。

リファクタリングとフィーチャー作業を分離してください。 既存コードをリファクタリングして新しい動作を追加する変更は2つの変更です — 別々に提出してください。小さなクリーンアップ（変数名の変更）はレビュアーの判断で含められます。

変更の説明

すべての変更にはバージョン管理履歴で単体で成立する説明が必要です。

1行目： 短く、命令形で、単体で成立。「削除する」ではなく「Delete the FizzBuzz RPC」。履歴を検索する人がdiffを読まずに変更を理解できるほど有益でなければなりません。

本文： 何が変更され、なぜか。コードに見えないコンテキスト、決定、推論を含めます。バグ番号、ベンチマーク結果、または設計ドキュメントへのリンク。アプローチの不足がある場合はそれを認める。

アンチパターン： 「Fix bug」、「Fix build」、「Add patch」、「Moving code from A to B」、「Phase 1」、「Add convenience functions」。

レビュープロセス

ステップ1：コンテキストを理解する

コードを見る前に、意図を理解します：

- この変更は何を達成しようとしているか？
- どの仕様またはタスクを実装しているか？
- 期待される動作の変更は何か？

ステップ2：テストを最初にレビューする

テストは意図とカバレッジを示します：

- 変更のテストは存在するか？
- 実装詳細ではなく動作をテストしているか？
- エッジケースがカバーされているか？
- テストに説明的な名前があるか？
- コードが変更されたときに回帰をキャッチするか？

ステップ3：実装をレビューする

5つの軸を念頭に置いてコードを確認します：

変更されたファイルごとに：
1. 正確性：このコードはテストが言うことをしているか？
2. 可読性：ヘルプなしにこれを理解できるか？
3. アーキテクチャ：これはシステムに適合しているか？
4. セキュリティ：脆弱性はないか？
5. パフォーマンス：ボトルネックはないか？

ステップ4：指摘を分類する

すべてのコメントに重大度ラベルを付けて、著者が必須と任意を区別できるようにします：

接頭辞	意味	著者の対応
(接頭辞なし)	必須の変更	マージ前に対応する必要がある
Critical:	マージをブロック	セキュリティ脆弱性、データ損失、機能破損
Nit:	小さい、任意	著者は無視可能 — フォーマット、スタイル設定
Optional: / Consider:	提案	検討する価値があるが必須ではない
FYI	情報のみ	対応不要 — 将来のリファレンス用コンテキスト

これにより、著者はすべてのフィードバックを必須と見なし、任意の提案に時間を浪費することを防ぎます。

ステップ5：検証を確認する

著者の検証ストーリーをチェックします：

- どのテストが実行されたか？
- ビルドは成功したか？
- 変更は手動でテストされたか？
- UIの変更のスクリーンショットはあるか？
- 変更前と変更後の比較があるか？

マルチモデルレビューパターン

異なるレビュー観点に異なるモデルを使用します：

モデルAがコードを作成
    │
    ▼
モデルBが正確性とアーキテクチャをレビュー
    │
    ▼
モデルAがフィードバックに対応
    │
    ▼
人間が最終判断

これにより、単一モデルが見落とす可能性のある問題をキャッチします — 異なるモデルは異なる盲点を持ちます。

レビューエージェント用のプロンプト例：

正確性、セキュリティ、およびプロジェクト規約の遵守についてこのコード変更をレビューしてください。
仕様では[X]と言っています。変更は[Y]すべきです。
Critical、Important、Suggestionとしてすべての問題にフラグを付けてください。

デッドコードの衛生管理

リファクタリングまたは実装変更後、孤立したコードをチェックします：

到達不可能またはて未使用のコードを特定する
明示的にリストする
削除する前に質問する： 「これらの未使用要素を削除すべきか：[リスト]？」

デッドコードを放置しないでください — 将来の読み手やエージェントを混乱させます。確信のないものを黙って削除しないでください。疑わしい場合は、質問してください。

デッドコード特定：
- formatLegacyDate() in src/utils/date.ts — formatDate()で置換
- OldTaskCard component in src/components/ — TaskCardで置換
- LEGACY_API_URL constant in src/config.ts — 参照なし
→ これらを削除しても安全か？

レビュー速度

遅いレビューはチーム全体をブロックします。レビューにコンテキストスイッチする費用は、他人に課すられる待機費用より少ないです。

1営業日以内に対応する — これは目標ではなく最大値です
理想的なペース： レビューリクエストが到着した直後に対応します（深い集中作業の場合を除く）。典型的な変更は1日で複数のレビューラウンドを完了すべきです
最終承認の迅速さより個別対応の迅速さを優先する。 迅速なフィードバックは複数ラウンド必要でも不満を軽減します
大きな変更： 1つの大規模な変更セットをレビューするのではなく、著者に分割するよう求めます

意見の相違への対処

レビューの相違を解決する場合、以下の階層を適用します：

技術的事実とデータは意見と設定を上回ります
スタイルガイドはスタイル問題の絶対的な権限です
ソフトウェア設計は個人的な設定ではなくエンジニアリングの原則で評価する必要があります
コードベースの一貫性は全体的な品質を低下させない場合は受け入れられます

「後で片付けます」は受け入れないでください。 経験上、延期されたクリーンアップはめったに行われません。真の緊急時を除き、提出前のクリーンアップを要求します。このコード変更で対応できない周囲の問題がある場合、自己割り当てでバグをファイリングすることを要求します。

レビューの誠実さ

自分、別のエージェント、または人間が書いたコードをレビューする場合：

ゴム判を押さないでください。 レビューの証拠なしに「LGTM」は誰の役にも立ちません。
本当の問題を和らげないでください。 「これは小さな懸念かもしれません」と言うのは、本番環境に当たるバグがある場合は不誠実です。
可能な場合は問題を定量化します。 「このN+1クエリはリスト内の項目ごとに約50msを追加します」は「これは遅い可能性があります」より良いです。
明らかな問題があるアプローチに異議を唱えます。 同調圧力はレビューの失敗モードです。実装に問題がある場合、直接それを言い、代替案を提案してください。
オーバーライドを優雅に受け入れます。 著者が全文脈を持ち、異議があれば、彼らの判断を尊重します。コードにコメントし、人にはコメントしません — 個人的な批評を自分自身へのコード焦点に再構成します。

依存関係の規律

コードレビューの一部は依存関係レビューです：

依存関係を追加する前に：

既存スタックでこれを解決できるか？（しばしばできます。）
依存関係はどのくらい大きいか？（バンドルインパクトをチェック。）
アクティブにメンテナンスされているか？（最後のコミット、オープンイシューをチェック。）
既知の脆弱性があるか？（npm audit）
ライセンスは何か？（プロジェクトと互換性がある必要がある。）

ルール： 新しい依存関係より標準ライブラリと既存ユーティリティを優先します。すべての依存関係は負債です。

レビューチェックリスト

## レビュー：[PR/変更タイトル]

### コンテキスト
- [ ] この変更が何をしてなぜかを理解しています

### 正確性
- [ ] 変更は仕様/タスク要件と一致する
- [ ] エッジケースが処理されている
- [ ] エラーパスが処理されている
- [ ] テストが変更を適切にカバーしている

### 可読性
- [ ] 名前が明確で一貫性がある
- [ ] ロジックが直感的
- [ ] 不要な複雑さがない

### アーキテクチャ
- [ ] 既存パターンに従う
- [ ] 不要な結合または依存関係がない
- [ ] 適切な抽象化レベル

### セキュリティ
- [ ] コードにシークレットがない
- [ ] 境界で入力が検証されている
- [ ] インジェクション脆弱性がない
- [ ] 認証チェックが行われている
- [ ] 外部データソースは信頼できないものとして扱われている

### パフォーマンス
- [ ] N+1パターンがない
- [ ] 無限操作がない
- [ ] リストエンドポイントにペーグネーションがある

### 検証
- [ ] テストが合格する
- [ ] ビルドが成功する
- [ ] 手動検証が行われている（該当する場合）

### 評決
- [ ] **Approve** — マージの準備ができました
- [ ] **Request changes** — 問題に対応する必要があります

よくある言い訳

言い訳	現実
「機能するから、十分です」	読みづらく、安全でなく、アーキテクチャ的に間違っているコードは複雑化する負債を生成します。
「自分が書いたから正しいと確かです」	著者は自分の仮定に盲目です。すべての変更は別の視点の恩恵を受けます。
「後でクリーンアップします」	後は来ません。レビューは品質ゲートです — 使用してください。マージ後ではなく前のクリーンアップを要求します。
「AI生成コードはおそらく問題ありません」	AIコードは少なくとも、より詳しくスクラッチされる必要があります。確信を持ち、間違っていても表面的です。
「テストが合格すれば、大丈夫です」	テストは必須ですが十分ではありません。アーキテクチャ問題、セキュリティ問題、可読性の懸念をキャッチしません。

危険信号

レビューなしでマージされたPR
テストが合格するかどうかだけをチェックするレビュー（他の軸を無視）
実際のレビューの証拠なしの「LGTM」
セキュリティに焦点を当てたレビューなしのセキュリティ敏感な変更
「適切にレビューするには大きすぎる」大きなPR（分割してください）
バグ修正PRに回帰テストがない
重大度ラベルなしのレビューコメント — 必須と任意が不明確
「後で修正します」を受け入れる — それは決して起こりません

検証

レビュー完了後：

すべてのCriticalな問題が解決されている
すべてのImportantな問題が解決されているか、正当化で明示的に延期されている
テストが合格する
ビルドが成功する
検証ストーリーが文書化されている（何が変更され、どのように検証されたか）

ライセンス: MIT(寛容ライセンスのため全文を引用しています) · 原本リポジトリ

code-review-and-quality

SKILL.md 本文