hacomono TECH BLOG

フィットネスクラブやスクールなどの顧客管理・予約・決済を行う、業界特化型SaaS「hacomono」を提供する会社のテックブログです！

AI生成レビューの"過剰な正しさ"にどう向き合うか

はじめに

hacomono でプロダクトエンジニアやってます、renren です。普段はRuby on Railsでバックエンド開発をしています。
記事を開いていただいてありがとうございます！


早速ですが、コードレビューどうやって行なっていますか？


私はClaude Codeのカスタムスキルでコードレビューを自動化しています。プルリクエスト（以降「PR」）を作ったらスキルを実行するだけで、レイヤーごとの責務分離やセキュリティ・パフォーマンス・E2E影響といった観点からコードをチェックし、結果をMarkdownファイルに構造化して出力してくれます。


これにより手動レビューでは見落としがちなエッジケースやセキュリティリスクを機械的に洗い出せます。また、チームではCodeRabbitも活用しています。


ただ、使っていてわかったのは、AIレビューの指摘には「本当に有用な指摘」と「表面的な知識に基づく過剰な指摘」が混ざっているということです。


この記事では、実際のPRレビューで遭遇した具体的なエピソードを通じて、AIレビューの指摘をどう受け止めるべきかを考えていきます。

AIの過剰な指摘

upsert_all(on_duplicate: :skip) を使ったところ、AIレビューでこんな指摘が来ました。

upsert_allの on_duplicate: :skip オプションはRails 7.1の公開ドキュメントに記載がありません。未公開APIの使用はランタイムエラーの可能性があるため、代替手段を検討してください。

一見もっともらしく、ドキュメントに載っていないAPIを使うのは確かにリスクがありますよね。

実際の調査結果

Railsのソースコードを確認してみると activerecord/lib/active_record/persistence.rbに upsert_allの定義があり、on_duplicateパラメータは公開メソッドの引数として明確に定義されていることが分かりました。さらに activerecord/lib/active_record/insert_all.rbを見ると:skipは明示的にハンドリングされていました。

# Rails内部実装（簡略化）
def initialize(model, inserts, on_duplicate:, ...)
  # on_duplicate: :skip は ON DUPLICATE KEY の無視に変換される
end

ドキュメントの記載が薄いのは事実ですが、「ドキュメントに記載がない = 未公開API」というのは間違いであり、ソースを読めばすぐわかる話でした。

コードベースの前例

プロジェクト内を検索すると、既存コードで全く同じパターンが使われていました。

# 既存の実装
def bulk_create(user, parent_id, item_ids, start_at = nil)
  records = item_ids.map do |item_id|
    record = {
      item_id:,
      parent_id:,
      created_by: user.id,
      updated_by: user.id,
    }
    record[:start_at] = start_at if start_at
    record
  end
  entity_class.upsert_all(records, on_duplicate: :skip)
end

既に採用しているパターンに「未公開APIだから使うな」と警告を出すのは、コードベースのコンテキストを読めていない気がしますね。
（既存の実装が100%正というわけでもないですが...）

結論として、この指摘はスルーしました。明示的に指示しなければ、Claude Codeがgemのソースコードまで自発的に読みに行くとは限りません。また、学習データのカットオフにより最新のRails内部実装を正確に把握していない場合もあります。

一方で、AIレビューが本当のバグを見つけた話

過剰な指摘がある一方で、AIレビューが実際のバグや設計上の問題を見つけたケースもあります。

有用な指摘1: 複合ユニークインデックスの欠如

(user_id, post_id) の組み合わせに対するユニークインデックスがありません。アプリケーションレベルの重複チェックだけでは、並行リクエストで重複レコードが作成される可能性があります。

正しいですね。アプリケーションコードで 既存レコードを除外する重複チェックをしていても、2つのリクエストが同時に来ればレースコンディションが起きます。マイグレーションで複合ユニークインデックスを追加しました。

有用な指摘2: トランザクション外の lock!

lock! がトランザクション外で呼ばれています。lock! はSELECT FOR UPDATEを発行しますが、トランザクション外では即座にロックが解放されるため、排他制御として機能しません。

実際のコードを見ると、バリデーション処理の中で lock! が呼ばれていました。

# バリデーション処理（トランザクションA）
def validate_transfer
  transfer.lock!  # ← ここでロックを取得するが...
  # バリデーション終了とともにトランザクションが終わり、ロックも解放される
end

# メイン処理（トランザクションB）
def execute_transfer
  # ← この時点では既にロックがない
  transfer.update!(status: :completed)
end

フレームワークの設計上、バリデーションとメイン処理が別トランザクションで実行される構造だったため、lock!で取得したロックはバリデーション終了時点で解放されてしまいます。これは設計上の問題で、修正の方向性を検討するきっかけになりました。

同じ技術要素への真逆の指摘

「AIの過剰な指摘」章で紹介した指摘で、CodeRabbit は upsert_all(on_duplicate: :skip) に対して「未公開APIだから使うな」と指摘しました。
一方、lock! の問題に対する改善策として、Claude Codeの自作レビュースキル（プロジェクトのコードベースをコンテキストとして渡している）は、こう提案しました。

lock! を外して upsert_all(on_duplicate: :skip) に変更することで、排他制御の問題を解消しつつ、冪等性のある実装になります。

同じ upsert_all(on_duplicate: :skip) という技術要素に対して、あるAIは「使うな」と言い、別のAIは「使うべき」と言ってきます。
この違いは何でしょうか。コードベースのコンテキストを理解しているかどうかだと思います。
upsert_all(on_duplicate: :skip) について以下の3点のコンテキストを持っていれば、これは「避けるべきもの」ではなく「積極的に活用できるもの」と判断できると思います。

  • プロジェクト内で既に使われているパターンであること
  • RDBMSレベルの冪等性が得られること
  • ロック設計の代替として有効であること


過剰な指摘と有用な指摘を見分けるポイント

これまでの話から、AIレビューの指摘を評価する基準を整理してみます。

コードベースの前例があるか

実装が古いパターンもあるため、一概には言えず悩ましいですが、
一次フィルターとしては同じパターンが既にプロジェクト内で使われていれば、それはチームの慣習と捉えていいと思います。
AIがそのパターンに警告を出しても、コードベースを検索して前例を確認すれば判断できます。

# 同じパターンが既に使われているか確認
grep -r "upsert_all.*on_duplicate.*skip" .
指摘の根拠を検証可能か

OSSであれば「ドキュメントに記載がない」は根拠としては弱く、実装を読めば判断できます。「ソースコードのどの部分が問題か」まで踏み込んでいない指摘は、疑ってかかるべきです。
AIレビューの指摘を鵜呑みにしないために、最近やっているのが「別のClaude Codeのセッションで検証する」という方法です。
例えば upsert_all(on_duplicate: :skip) の指摘に対して、別セッションで「Railsの upsert_all で on_duplicate: :skip は安全に使えるか？ソースコードベースで教えて」と聞くと、こんな回答が返ってきます。

Railsのソースコードを確認すると、on_duplicate は InsertAll クラスの公開コンストラクタ引数として定義されています。:skip は ON DUPLICATE KEY の処理で明示的にハンドリングされており、ランタイムエラーは発生しません。ただし、ドキュメントでの記載は on_duplicate: :update のパターンが中心であり、:skip の説明は薄いです。

「半分正しく、半分間違っている」というニュアンスの回答です。APIとしては安全、という判断材料が得られました。
AIの指摘をフィルタリングするために、もう1つのAIを使うというメタ的な構造ですが、実用上はうまく機能しています。ポイントは「ソースコードベースで」と明示的に指示することで、ドキュメントだけでなく実装まで確認させることです。

指摘の種類ごとのAIの精度

AIレビューの真価は「ロジックの穴」を見つけることにあると考えています。これまでの経験から、指摘の種類ごとにAIの精度を整理してみました。

指摘の種類 AIの精度 例
バグ（ロジックの穴） 高い 重複IDの問題、インデックス欠如、ロック設計の問題
エッジケース 高い 並行リクエスト、NULL値、空配列
セキュリティ 中程度 SQLインジェクション、入力検証の漏れ
設計判断 低い 責務分割の粒度、API選択、アーキテクチャ
スタイル 低い メソッド名、コメントの有無、コードの並び順

バグやエッジケースの指摘は積極的に受け入れ、設計判断やスタイルの指摘はコンテキストを踏まえて取捨選択する。これが自分なりのAIレビューとの付き合い方だと思っています。

まとめ

AIレビューは「大量の目を持つ一次フィルター」であって、完璧なレビュアーではないと思っています。トランザクション外の lock!のように、書いた本人が気づきにくいバグを見つける能力は高いですが、チームが意図的に採用しているパターンやメソッドの適切なサイズ感は、コードだけを見ても判断できません。
個人的には、AIレビューの指摘を全部直すのは微妙だと思っています。それをやると、コードベースは「教科書的に正しいが、チームの文脈から浮いたもの」になってしまいます。バグの指摘は真剣に受け止め、設計の指摘はコンテキストと照らし合わせます。迷ったら別のAIセッションでソースコードベースの検証をかけます。それくらいのスタンスでちょうどいいと思っています。


💁 関連記事