code-review-and-quality

コードレビューと品質

概要

品質ゲートを設けた多次元のコードレビューです。すべての変更はマージ前にレビューします。例外はありません。レビューでは、正確性、可読性、アーキテクチャ、セキュリティ、パフォーマンスの5つの観点を確認します。

承認基準： 完璧でなくても、コード全体の健全性が確実に向上する変更は承認します。完璧なコードは存在しません。目標は継続的な改善です。自分ならまったく同じ書き方をしないという理由で、変更を止めないでください。コードベースが改善され、プロジェクトの規約に従っているなら、承認してください。

使用するタイミング

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

5つの観点からのレビュー

すべてのレビューで、次の観点からコードを評価します。

1. 正確性

コードは、うたわれているとおりに動作しますか？

• 仕様やタスクの要件に合致していますか？
• エッジケース（null、空の値、境界値）は処理されていますか？
• エラーパス（正常系だけではなく）は処理されていますか？
• すべてのテストに合格しますか？テストは実際に適切な内容を検証していますか？
• 1つずれのエラー、競合状態、状態の不整合はありませんか？

2. 可読性とシンプルさ

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

• 名前は内容を適切に表し、プロジェクトの規約に沿っていますか？（文脈のない temp、data、result は避けます）
• 制御フローは分かりやすいですか？（入れ子の三項演算子や深いコールバックは避けます）
• コードは論理的に整理されていますか？（関連するコードがまとまり、モジュールの境界が明確ですか）
• 単純化できる「巧妙な」小技はありませんか？
• もっと少ない行数で実現できますか？（100行で十分なところに1000行も使っているなら問題です）
• 抽象化は、その複雑さに見合う価値がありますか？（3つ目のユースケースが現れるまでは一般化しないでください）
• 一目で分からない意図を明確にするコメントは役立ちますか？（ただし、自明なコードにはコメントを書かないでください）
• no-op 変数（_unused）、後方互換性のためのシム、// removed コメントなど、不要なコードの残骸はありませんか？
• 無関係な処理フローに新しい条件分岐を付け足していませんか？ これは軽微な指摘ではなく、設計上の問題です。既存の処理フローに絡ませるのではなく、ロジックを専用のヘルパー、状態、またはポリシーに分離してください。
• 同じ構造に対する条件分岐が繰り返されていませんか？ これは、モデルやディスパッチャーが不足している兆候です。「一時的な」分岐は、たいてい恒久的な負債になります。

3. アーキテクチャ

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

• 既存のパターンに従っていますか、それとも新しいパターンを導入していますか？新しい場合、その導入は正当化されていますか？
• モジュールの境界は適切に保たれていますか？
• 共通化すべきコードの重複はありませんか？
• 依存関係は適切な方向に流れていますか？（循環依存はありませんか）
• 抽象化のレベルは適切ですか？（過剰な設計でも、結合しすぎてもいませんか）
• このリファクタリングは複雑さを減らしていますか、それとも移動させているだけですか？ 変更を理解するために読者が把握しておく必要がある概念の数を数えてください。「よりクリーンな」バージョンでもその数が変わらないなら、クリーンになったとは言えません。同じロジックを再集約するだけの方法よりも、分岐、モード、レイヤー全体がなくなる再構成を優先してください。抽象化を磨くより、削除することを優先してください。
• 機能固有のロジックが共有モジュールや汎用モジュールに漏れ出していませんか？ ロジックは本来のレイヤーに置き、類似した重複ヘルパーを作るのではなく、既存の正式なヘルパーを再利用し、アーキテクチャのずれを定着させないでください。
• 型の境界は明示されていますか？ 不必要な any／unknown／オプショナル型／キャストや、不明確な不変条件を覆い隠す暗黙のフォールバックを見直してください。境界を明示すると、周囲の制御フローがよりシンプルになることがよくあります。

4. セキュリティ

セキュリティに関する詳細なガイダンスは、security-and-hardeningを参照してください。この変更によって脆弱性が生じていませんか？

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

5. パフォーマンス

プロファイリングと最適化に関する詳細は、performance-optimizationを参照してください。この変更によってパフォーマンスの問題が生じていませんか？

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

構造上の改善策

構造上の問題を指摘するときは、問題だけでなく、どのように変更するかも提案してください。「複雑です」とだけ述べるレビューでは、作成者はどうすればよいか分かりません。具体的なリファクタリング案を挙げてください。

• 条件分岐の連鎖を、型付きモデルまたは明示的なディスパッチャーに置き換えます。
• 重複する分岐をまとめて、より明確な単一のフローにします。
• それぞれが独立して読めるよう、オーケストレーションとビジネスロジックを分離します。
• 機能固有のロジックを共有モジュールから、その概念を所有するパッケージに移します。
• 独自の類似実装ではなく、標準のヘルパーを再利用します。
• 下流の分岐が不要になるよう、型の境界を明示します。
• API を明確にせず間接性を増やすだけの、処理をそのまま通すラッパーを削除します。
• ヘルパーを抽出するか、大きなファイルを役割に特化したモジュールに分割します。

同じ複雑さを別の場所に広げる方法より、構成要素を減らせる改善策を優先してください。

変更の規模

小さく焦点を絞った変更はレビューしやすく、マージも速く、安全にデプロイできます。次の規模を目安にしてください。

~100 lines changed   → Good. Reviewable in one sitting.
~300 lines changed   → Acceptable if it's a single logical change.
~1000 lines changed  → Too large. Split it.

差分のサイズだけでなく、ファイルサイズにも注意してください。 差分が小さくても、ファイルが適切な境界を超えてしまうことがあります。単一ファイルで合計約 1000 行（上記の変更行数の目安である約 1000 行とは別です）は、上限ではありませんが、確認が必要な一般的な目安です。変更によって、すでに大きなファイルが大幅に増える場合は、さらに追加する前にヘルパー、サブコンポーネント、モジュールを抽出できないか検討してください。分解してから追加します。

「1 つの変更」とは： 1 つのことに対処し、関連するテストを含み、提出後もシステムが機能する状態を保つ、単独で完結した変更です。機能全体ではなく、その一部分を指します。

変更が大きすぎる場合の分割方法：

方法	手順	適用場面
積み重ね	小さな変更を提出し、それを基に次の変更を始める	順次依存がある場合
ファイルグループ別	異なるレビュアーが必要なファイルグループごとに変更を分ける	複数領域にまたがる懸念事項
水平分割	まず共有コードやスタブを作成し、その後に利用側を追加する	レイヤードアーキテクチャ
垂直分割	機能を、フルスタックの小さなスライスに分割する	機能開発

大きな変更が許容される場合： ファイルの完全な削除や、自動化されたリファクタリングなど、レビュアーがすべての行ではなく意図だけを確認すればよい場合です。

リファクタリングと機能開発は分けてください。 既存コードのリファクタリングと新しい動作の追加を同時に行う変更は、2 つの変更にあたります。別々に提出してください。小さな整理（変数名の変更など）は、レビュアーの判断で含めてもかまいません。

変更内容の説明

すべての変更には、バージョン管理の履歴だけで内容が伝わる説明が必要です。

1 行目： 短く、命令形で、それだけで意味が伝わるようにします。「FizzBuzz RPC を削除する」は適切ですが、「FizzBuzz RPC を削除しています」は適切ではありません。差分を読まなくても、履歴を検索した人が変更内容を理解できるだけの情報を含めてください。

本文： 何を変更するのか、なぜ変更するのかを記載します。コード自体からはわからない背景、判断、理由を含めてください。関連する場合は、バグ番号、ベンチマーク結果、設計ドキュメントへのリンクを記載します。アプローチに欠点がある場合は、その点も明記してください。

アンチパターン： 「バグを修正」「ビルドを修正」「パッチを追加」「コードを A から B に移動」「フェーズ 1」「便利な関数を追加」

レビュープロセス

ステップ 1：背景を把握する

コードを見る前に、変更の意図を把握します。

- What is this change trying to accomplish?
- What spec or task does it implement?
- What is the expected behavior change?

ステップ 2：まずテストを確認する

テストから、意図と網羅範囲がわかります。

- Do tests exist for the change?
- Do they test behavior (not implementation details)?
- Are edge cases covered?
- Do tests have descriptive names?
- Would the tests catch a regression if the code changed?

最後の質問には、コードを読むのではなく、実験して答えてください。変更によって追加された条件を 1 つ反転させ（否定を外す、&& を || に置き換えるなど）、テストスイートを実行してから、コピーからファイルを復元します。変更後もテストが成功する場合は問題点です。不足しているテストケースを特定してください。プロジェクト全体のミューテーションスコアについては、constraint-driven-development を参照してください。

ステップ 3：実装を確認する

次の 5 つの観点を意識しながらコードを確認します。

For each file changed:
1. Correctness: Does this code do what the test says it should?
2. Readability: Can I understand this without help?
3. Architecture: Does this fit the system?
4. Security: Any vulnerabilities?
5. Performance: Any bottlenecks?

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

作者が必須事項と任意事項を区別できるよう、すべてのコメントに重大度を付けます。

接頭辞	意味	作者の対応
(接頭辞なし)	必須の変更	マージ前に対応する必要があります
Critical:	マージを阻止する問題	セキュリティ上の脆弱性、データ損失、機能の破損
Nit:	軽微で任意	作者は対応しなくてもかまいません。書式やスタイルの好みなど
Optional: / Consider:	提案	検討する価値はありますが、必須ではありません
FYI	情報のみ	対応は不要です。今後の参考情報です

これにより、作者がすべてのフィードバックを必須事項として扱い、任意の提案に時間を費やすことを防げます。

重要な点から伝えてください。 指摘事項は影響度の高い順に並べます。まず正確性とセキュリティ、次に構造的なリグレッションや見落とされた簡素化、それ以外の順です。実際の問題を見た目だけの細かな指摘に埋もれさせないでください。確信度の高い指摘を少数挙げるほうが、長いリストよりも効果的です。構造上の問題が 1 つ、細かな指摘が 10 個あるなら、レビューの中心となるのは構造上の問題です。

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

作者がどのように検証したかを確認します。

- What tests were run?
- Did the build pass?
- Was the change tested manually?
- Are there screenshots for UI changes?
- Is there a before/after comparison?

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

レビューの観点ごとに異なるモデルを使います。

Model A writes the code
    │
    ▼
Model B reviews for correctness and architecture
    │
    ▼
Model A addresses the feedback
    │
    ▼
Human makes the final call

モデルごとに見落としやすい点が異なるため、単一のモデルでは見つけられない問題も発見できます。

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

Review this code change for correctness, security, and adherence to
our project conventions. The spec says [X]. The change should [Y].
Flag any issues as Critical, Required, Optional, or Nit.

不要なコードの整理

リファクタリングや実装変更の後は、孤立したコードがないか確認します。

1. 到達不能または未使用になったコードを特定します
2. そのコードを明示的に列挙します
3. 削除する前に確認します：「現在未使用の次の要素を削除しますか：[一覧]」

不要なコードを放置しないでください。将来の読者やエージェントを混乱させます。ただし、確信がないものを黙って削除してはいけません。迷ったら、確認してください。

DEAD CODE IDENTIFIED:
- formatLegacyDate() in src/utils/date.ts — replaced by formatDate()
- OldTaskCard component in src/components/ — replaced by TaskCard
- LEGACY_API_URL constant in src/config.ts — no remaining references
→ Safe to remove these?

レビューの速度

レビューが遅れると、チーム全体の作業が滞ります。レビューに切り替えるためのコストより、ほかの人に発生する待ち時間のほうが大きくなります。

• 1 営業日以内に返信します。 これは目標ではなく、最長の期限です
• 理想的なペース： 集中してコーディングしている最中でなければ、レビュー依頼が届いてから早めに返信します。通常の変更であれば、1 日で複数回のレビューを完了できるはずです
• 最終承認を急ぐより、個々の返信を早く行うことを優先します。 複数回のやり取りが必要でも、迅速なフィードバックは不満を軽減します
• 大規模な変更： 非常に大きな変更セットを一度にレビューするのではなく、作者に分割を依頼します

意見の相違への対処

レビューで意見が分かれたときは、次の優先順位に従って解決します。

1. 技術的な事実とデータを意見や好みより優先します
2. スタイルガイドは、スタイルに関する事項の絶対的な基準です
3. ソフトウェア設計は、個人の好みではなく、エンジニアリングの原則に基づいて評価します
4. 全体的な健全性を損なわないなら、コードベースの一貫性を優先してもかまいません

「あとで整理します」を認めないでください。 これまでの経験から、先送りされた整理が実行されることはほとんどありません。本当の緊急事態でない限り、提出前に整理を求めてください。この変更で周辺の問題に対処できない場合は、担当者を自分に割り当てたバグチケットを作成するよう求めてください。

レビューにおける誠実さ

自分、別のエージェント、または人間が書いたコードをレビューするときは、次の点に留意してください。

• 内容を確認せずに承認しないでください。 根拠のない「LGTM」は誰の役にも立ちません。
• 実際の問題をぼかさないでください。 本番環境に影響するバグを「些細な懸念かもしれません」と表現するのは不誠実です。
• 可能な限り問題を定量化してください。 「この N+1 クエリは一覧の項目ごとに約50msの遅延を加えます」は、「遅くなる可能性があります」より適切です。
• 明らかな問題がある手法には異議を唱えてください。 レビューにおいて、相手に迎合することは失敗につながります。実装に問題があるなら、率直に指摘し、代替案を提案してください。
• 上書きを穏やかに受け入れてください。 作者が全体像を把握したうえで異なる判断をした場合は、その判断に委ねてください。人ではなくコードについてコメントし、個人への批判はコード自体に焦点を当てた表現に改めてください。

依存関係の管理

コードレビューには、依存関係のレビューも含まれます。

依存関係を追加する前に、次の点を確認してください。

1. 既存のスタックで解決できますか？（多くの場合、解決できます。）
2. 依存関係の規模はどの程度ですか？（バンドルへの影響を確認してください。）
3. 継続的にメンテナンスされていますか？（最終コミットや未解決のイシューを確認してください。）
4. 既知の脆弱性はありますか？（npm audit）
5. ライセンスは何ですか？（プロジェクトと互換性がある必要があります。）

原則： 新しい依存関係よりも、標準ライブラリや既存のユーティリティを優先してください。依存関係は増えるごとに負担になります。

既存の依存関係のアップグレードも、ほかの変更と同様にコード変更です。特にリスクが高いのは、「依存関係を更新」といったメッセージでまとめてマージされるアップグレードです。同じ厳密さでレビューしてください。

1. バージョン番号だけでなく、変更履歴を読んでください。 Semver はメンテナーが守っていない可能性のある約束です。「パッチ」更新でも動作が変わることがあります。メジャーアップグレードでは、移行ノートを読み、何が壊れるかを確認してください。
2. 変更ごとに依存関係を1つにしてください。 個別に（または関連する小さなグループごとに）アップグレードしてマージしてください。一括更新でビルドが壊れると、どのパッケージが原因か分からなくなります。単一パッケージの変更なら原因が明確になり、簡単に元に戻せます。
3. テストを判断基準にしてください。 アップグレードの検証では、インストールできたことではなく、アップグレードの前後でテストスイートがすべて成功することを確認してください。依存関係の動作を検証するテストが十分でないなら、本当の問題はその不足です。先にテストを追加してください。
4. 推移的な依存関係のグラフにも注意してください。 インストールされているパッケージの多くは、誰も直接選んだものではありません。package.json だけでなく、ロックファイルの差分もレビューしてください。直接依存関係を1つ更新するだけで、数十個の間接依存関係が追加・更新されることがあります。
5. ロックファイルを正しく扱ってください。 ロックファイルをコミットし、その差分をレビューし、手作業で編集しないでください。実際に出荷される内容を固定するのはロックファイルです。

npm audit の検出結果やサプライチェーンのリスク（タイポスクワッティング、メンテナーの侵害）をトリアージするには、security-and-hardening スキルに従ってください。このセクションではアップグレードのワークフローを扱い、もう一方ではセキュリティ上の判断を扱います。

レビューチェックリスト

## Review: [PR/Change title]

### Context
- [ ] I understand what this change does and why

### Correctness
- [ ] Change matches spec/task requirements
- [ ] Edge cases handled
- [ ] Error paths handled
- [ ] Tests cover the change adequately

### Readability
- [ ] Names are clear and consistent
- [ ] Logic is straightforward
- [ ] No unnecessary complexity

### Architecture
- [ ] Follows existing patterns
- [ ] No unnecessary coupling or dependencies
- [ ] Appropriate abstraction level
- [ ] Refactors reduce complexity rather than relocate it
- [ ] No feature logic in shared modules; file stays within a healthy size

### Security
- [ ] No secrets in code
- [ ] Input validated at boundaries
- [ ] No injection vulnerabilities
- [ ] Auth checks in place
- [ ] External data sources treated as untrusted

### Performance
- [ ] No N+1 patterns
- [ ] No unbounded operations
- [ ] Pagination on list endpoints

### Verification
- [ ] Tests pass
- [ ] Build succeeds
- [ ] Manual verification done (if applicable)

### Verdict
- [ ] **Approve** — Ready to merge
- [ ] **Request changes** — Issues must be addressed

関連項目

• セキュリティレビューの詳細なガイダンスについては、../../references/security-checklist.md を参照してください
• パフォーマンスレビューの確認項目については、../../references/performance-checklist.md を参照してください

よくある合理化

合理化	実態
「動いているから、これで十分」	読みにくい、安全でない、またはアーキテクチャ上誤ったコードは、積み重なっていく負債を生みます。
「自分で書いたのだから、正しいと分かっている」	作成者は自分自身の思い込みに気づきにくいものです。どのような変更でも、別の人の目で確認することが有益です。
「あとで整理する」	あとは来ません。レビューが品質のゲートです。活用してください。マージ後ではなく、マージ前に整理を必須にしてください。
「AI が生成したコードなら、たぶん問題ない」	AI のコードは、精査を減らすのではなく、増やす必要があります。誤っていても、自信に満ちていて、もっともらしく見えるからです。
「テストが通ったから、問題ない」	テストは必要ですが、それだけでは不十分です。アーキテクチャ上の問題、セキュリティ上の問題、読みやすさに関する懸念は検出できません。
「リファクタリングでコードがすっきりする」	複雑さを移動させても、減らしたことにはなりません。読み手が同じ数の概念を把握する必要があるなら、構造は改善されていません。分岐がなくなる形を探してください。
「このファイルに少し追加するだけ」	小さな差分でも、ファイルを健全なサイズの上限を超えさせたり、無関係な処理フローに分岐を付け足したりすることがあります。差分の大きさではなく、変更後の構造を評価してください。
「バージョンを上げるだけ」	バージョンアップは、自分で書いていない動作変更です。変更履歴を読んでください。semver でも破壊的変更がないとは限りません。
「時間を節約するために、すべてまとめてアップグレードする」	一括アップデートでビルドが壊れると、どのパッケージが原因か分かりにくくなります。変更ごとに依存関係を1つずつ更新すれば、原因の特定も取り消しも容易になります。

危険信号

• レビューを受けずにマージされた PR
• テストが通るかどうかだけを確認するレビュー（他の観点を無視）
• 実際にレビューした証拠のない「LGTM」
• セキュリティ上重要な変更に対して、セキュリティに重点を置いたレビューがない
• 「適切にレビューするには大きすぎる」大規模な PR（分割してください）
• バグ修正 PR に回帰テストがない
• 重大度ラベルのないレビューコメント（必須事項と任意事項の区別が不明確になる）
• 「あとで修正する」を受け入れること（実行されることはありません）
• 読み手が把握する必要のある概念の数を減らさず、コードを移動するだけのリファクタリング
• すでに大きいファイルを分割せず、さらに肥大化させる変更
• 無関係なコードパスに新しい条件分岐が散在すること（抽象化が不足しています）
• 既存の標準ヘルパーと重複する独自ヘルパー、または共有モジュールに置かれた機能ロジック
• 変更履歴を確認せず、パッケージごとに分離もされていない、一括の「依存関係のアップデート」PR
• 手作業で編集された、コミットされていない、または差分をレビューせずにマージされたロックファイルの変更

検証

レビューの完了後：

• すべての重大な問題が解決されている
• 「必須」（接頭辞なし）の変更がすべて解決されているか、理由を明示して延期されている
• テストが通る
• ビルドが成功する
• 検証内容が記録されている（何を変更し、どのように検証したか）
• 依存関係のアップグレードについて、変更履歴を確認し、パッケージごとに分離し、ロックファイルの差分をレビューしたうえで、すべてのテストが成功することを確認している

原則としてブロッカーとなる項目： 次の各項目について、表面化させて、よりシンプルな設計を提案してください。変更によって構造が実際に悪化する場合に限り、「必須」に引き上げてください。複雑さを減らさずに移動させるリファクタリング、分割せずにファイルのサイズ上限を超えさせる変更、共有モジュールに追加された機能ロジック、既存の標準ヘルパーとほぼ重複するもの、不明確な不変条件を隠す暗黙のフォールバック。