この記事はなに？
業務のAI化を見据えて、コードレビューの観点リストを作ってみました。
このリストは、Claude Codeなどのコーディング向けAIエージェントに読み込ませることを想定しているもののため、できるだけ指標が主観的にならないようにしています。
また、実験的に自分のプロジェクトで試していき、結果に応じて適宜更新していくことを想定しています。
観点ここから
原則
- コードレビューAIによるレビューの場合、原則的にコードレビューAIが指摘する観点に従う。このガイドラインに書いている内容は、付加的な観点として扱い、両者で矛盾する観点があれば、このガイドラインに書いてあるものが優先される
形式面のチェック
- プロジェクト内でプルリクエストやIssueチケットの形式が指定されている場合、それに適合しているか？
- プルリクエストのマージ後に残作業がある場合、それがプルリクエスト内に明記されているか？
- ソースコードや設定ファイルは、コメント等も含めすべて英語で記載されているか？
- README、設計ドキュメント、APIドキュメントなどが変更内容に合わせて更新されているか？
- 運用手順書やトラブルシューティングガイドが必要な場合、更新されているか？
機能要件への適合性
- プルリクエストのコメント、要件定義書と仕様書を読み、変更箇所が抜け漏れなく実装されているか？
- レビュー時にフォーマッタを通す（実装されていない場合は、その旨をレビューコメントとして記載する）
- レビュー時に単体テストを通す（テストスイート、静的解析ツールが実装されていない場合は、その旨をレビューコメントとして記載する）
- レビュー時に自動化されたE2Eテストを通す（実装されていない場合は、その旨をレビューコメントとして記載する）
- 可能な限り、レビュー時にローカル開発環境またはテスト環境にて簡単に動作を確認する。フロントエンドアプリケーションの場合はスクリーンショットを取る。バックエンドアプリケーションの場合はレスポンスを確認する（これらが難しい場合は、その旨をレビューコメントとして記載する）
非機能要件への適合性
設計全般
- 設計ガイドラインが別にある場合は、それに従っているか？
- SOLID原則を守っているか？
- 言語やフレームワークのベストプラクティスに従っているか？言語やフレームワークの機能を用いればできることを自力で実装しようとしていないか？
- 意図のわかりにくいコード（マジックナンバー、変数のわかりにくい命名など）や複雑なコード（深いネスト）を書いていないか？
- 必要のないコード（到達しないコードや不必要な設定、現状を表さないソースコードコメント、品質に寄与しない単体テストなど）を書いていないか？書いている場合は、積極的に更新/削除を提案する
- エラーハンドリングを確実に行っているか？異常系を握りつぶしていないか？異常の場合はログを確実に出力しているか？特に外部システムへのアクセスがある場合、外部システムから返されるエラーに対して適切なレスポンスやエラーログを返すようになっているか？
- 日時の処理が、タイムゾーンを考慮したものになっているか？また、年、月をまたいだ際に想定外の動作をしないか？
- 特殊文字やサロゲートペアなど、特殊な文字についても想定外の動作をしないか？
- データベースを用いるアプリケーションの場合、データベース制約（NOT NULL、UNIQUE、FOREIGN KEYなど）が適切に設定されているか？
- データベースを用いるアプリケーションの場合、楽観的ロック/悲観的ロックが適切に実装されているか？
- 後方互換性が担保されているか？UIやインタフェースを変更する場合は、それが破壊的変更になっていないか、なっている場合は、旧バージョンへの対応ができているかを確認する
- README、設計ドキュメント、APIドキュメントなどが変更内容に合わせて更新されているか？
単体テスト
- テストガイドラインが別にある場合は、それに従っているか？ない場合は作成する
- 単体テストカバレッジはC0 80%以上あるか？
- テスト対象となるメソッドが副作用を持たないか、持っていたとしても、依存関係を可能な限りメソッド外部に出せているか？
- Flakyなテスト（特に、現在時刻に依存するテスト、Sleep等を入れないと動作しないテスト、外部のシステムに依存するテスト）については、その点を必ず指摘し、適切な設計をレビューコメントとして記載する
- テストが外部ファイルやシステム等に依存する場合、テストダブルを用いたテストが可能になっているか？可能な場合、テストケースに必要十分なテストダブルが記載されているか？
- 意味のあるテストになっているか？テストケースの記述と、実際にアサーションの対象となる内容が一致しているか？本来テスト対象とすべきものまでテストダブル化していないか？
パフォーマンス
- パフォーマンス要件（SLO等）がある場合は、それに従っているか？
- 原則、全探索のロジック（指数時間の計算になるもの）は書かない
- 現実的でない量のメモリを確保する実装になっていないか？
- 並行処理やトランザクションが記載されている場合は、それが安全に行われているかどうか？。特にデッドロックや競合状態が起こりうる場合は、その点を必ず指摘する
- メモリリークの可能性がないか？
- データベースへのアクセスがある場合、典型的なクエリチューニング（N+1問題への対応、大量更新クエリへのバルクコミットなど）ができているか？
- ファイルハンドル、データベース接続、ネットワーク接続などが確実にクローズされているか？
- 外部システムへのアクセスがある場合、外部システムのレスポンス遅延や異常に対して対処しているか？アクセスのリトライ回数、タイムアウトまでの時間、またはサーキットブレーカーが適切に設定されているか？
- 想定外の負荷が来た場合、適切にエラーを返す、あるいは縮退運転ができる仕組みになっているか？
- 対象コードがサーバ設定の場合、必ず3台以上のサーバで構成され、冗長性が担保されているか？
- パフォーマンス（RPS、レイテンシなど）がログから観測可能になっているか？
セキュリティ
- セキュリティガイドラインが別にある場合は、それに従っているか？
- 認証、認可を実装する場合、適切に実装されているか？仕様の意図に反する実装や、抜け道を作る実装になっていないか？
- アプリケーション内で暗号化、あるいはハッシュ化を行う場合、アルゴリズムが危殆化していないか？適切な鍵を用いているか？
- パスワードやクレデンシャルなどの秘密情報をソースコード内やログ内に記載していないか？また、これらを用いる場合、容易に推測されない値になっているか？
- 依存するライブラリのバージョンが固定されているか？最新版かLTSのバージョン、かつ、すでに脆弱性が報告されているバージョンではないか？
- 入力値が適切にバリデーションされているか？ユーザや運用者からのどのような入力に対しても適切な応答またはエラーを返し、かつ想定外の動作をしないようになっているか？
- 技術的な情報（ライブラリやフレームワークのバージョンなど）が、ログやレスポンスを介してユーザから見えるようになっていないか？
- デバッグ用の設定やデフォルトの秘密情報が本番環境向けの設定で利用されていないか？
- テスト環境がインターネット外部からアクセスできるようになっていないか？
- その他、典型的な攻撃（特にOWASP Top 10に記載されているもの）への耐性があるか？
コンプライアンス
- 個人情報の取り扱いが適切（個人情報保護法やGDPRなどの規制に準拠している）か？
- データを保存するアプリケーションの場合、データの保持期間と削除ポリシーが実装されているか？
- 監査ログが必要な場合、適切に実装されているか？
- ライセンス（依存ライブラリを含む）が組織のポリシーに適合しているか？
ユーザビリティ / アクセシビリティ
- ユーザビリティについてのガイドライン（デザインシステムなど）がある場合、それに従っているか？
- ユーザへのUI、APIレスポンス、エラーメッセージやログは、その状況を正確に表すわかりやすいものになっているか？
観点ここまで
余談
昔コードレビューについて一考したことがありますが、
チームでコードレビューをしているときに考えていること
あの頃と比べるとコーディングも大きく変わったなと感じます。
AIで書くと、どうしてもレビューがボトルネックになりがちなので、コードのdiffを1行1行確認するというよりは、受け入れの基準をチケット等に記載して読み込ませたうえで、大まかな設計と単体テストのケースと結果をレビューした方がうまくいくような気がしますね（それでも結構しんどいのですが。。）。