AIの生成コードをレビューする観点 — 設計との整合・影響範囲・テスト
この記事で扱う問題
AI コーディングツールが書いたコードは、たいてい整っています。命名はそれらしく、型も付いていて、テストも一緒に書かれ、CI も通ります。人が書いたコードなら「書き方が雑なところに問題が潜んでいる」という手がかりがありますが、AI のコードは問題のある箇所も同じように整っています。
その結果、レビューで次のことが起きがちです。
- 差分が大きく、1行ずつ読むうちに集中が切れ、大事な箇所を流し読みしてしまう
- 動くし読みやすいので、既存の設計と違う書き方が入っていることに気づかない
- テストが通っていることを、要件を満たしていることの根拠にしてしまう
私は個人開発で、Spec と Plan を人が確かめてから AI に実装させています（開発の流れ）。チームの標準として運用した実績はまだありませんが、その中で、実装後のレビューで何を見るかの順序は固まってきました。この記事ではそれをまとめます。
判断
AI の生成コードは、次の順で確かめます。先に読む範囲を絞り、何を確かめるべきかをテストで知ってから、実装を読みます。
1. 差分の範囲を Plan と突き合わせる
コードを読む前に、変更されたファイルの一覧を見て、Plan に書いたものと比べます。見るのは次の点です。
- Plan にないファイルが変わっていないか（「ついでの」リファクタリング、共通クラスの修正）
- 公開しているメソッドのシグネチャや、API のレスポンスの形が変わっていないか。変わっていれば、呼び出し元をすべて確かめたか
- 既存のテストの期待値が書き換えられていないか
- マイグレーション、設定ファイル、
composer.jsonが変わっていないか
特に3つ目は、AI がテストを通すために、実装ではなくテストの側を直していることがあります。
- $this->assertSame('pending', $order->fresh()->status);
+ $this->assertSame('cancelled', $order->fresh()->status);
期待値の変更が要件の変更によるものなら問題ありません。ただ、それは Spec で決めたことのはずなので、Spec に書かれていない期待値の変更は、まず疑います。
2. 実装より先にテストを読み、受け入れ条件と対応させる
例えば、「未確定の注文をキャンセルできるようにする」という Spec を渡したとします。AI は次のようなテストを書いてきます。
public function test_注文をキャンセルできる(): void
{
$order = OrderModel::factory()->pending()->create();
$this->actingAs($order->user)
->postJson("/api/orders/{$order->id}/cancel")
->assertOk();
$this->assertSame('cancelled', $order->fresh()->status); // status を文字列のまま扱う前提。OrderStatus にキャストするなら OrderStatus::Cancelled と比べる
}
このテストは正しく、通ります。ただ、これは「実装がしたこと」を確かめているだけです。Spec の受け入れ条件には、次のものもあったはずです。
- 確定済みの注文はキャンセルできない
- 他人の注文はキャンセルできない
- キャンセル済みの注文をもう一度キャンセルしたらどうなるか
そこで、テスト名の一覧を受け入れ条件と1つずつ対応させます。テスト名を日本語で書いておくと、この対応がそのまま読めます。
public function test_確定済みの注文はキャンセルできない(): void
{
$order = OrderModel::factory()->confirmed()->create();
$this->actingAs($order->user)
->postJson("/api/orders/{$order->id}/cancel")
->assertConflict(); // OrderCannotBeCancelled を 409 に変換する前提
$this->assertSame('confirmed', $order->fresh()->status);
}
対応するテストがない条件は、実装もされていない可能性があります。3つ目のように Spec で決め忘れていた条件が見つかることもあり、そのときは AI に決めさせず、Spec に戻って人が決めます。
3. 既存の設計と同じ形になっているか
テストが要件を表していても、実装が既存の設計から外れていることがあります。同じ Spec に対して、次のような UseCase が出てきたとします。
final class CancelOrder
{
public function execute(string $orderId): void
{
$order = OrderModel::findOrFail($orderId);
if ($order->status === 'confirmed') {
throw new RuntimeException('確定済みの注文はキャンセルできません');
}
$order->status = 'cancelled';
$order->save();
}
}
動きますし、AI が書いた正常系のテストも通ります。一方で、上の「確定済みの注文はキャンセルできない」のテストは通りません。投げているのが OrderCannotBeCancelled ではなく RuntimeException なので、409 ではなく 500 になるからです。そもそも、既存の ConfirmOrder は、業務ルールを Laravel に依存しない場所に置く形で書かれています。この UseCase は、Eloquent を直接使い、「どの状態ならキャンセルできるか」のルールを Order ではなく UseCase に書いています。しかも条件が「確定済みでなければよい」なので、キャンセル済みの注文も通します。誰の注文かも確かめていないので、「他人の注文はキャンセルできない」も満たしていません。
既存の形に合わせると、ルールは Order に置かれます（Order のほかの部分は既存の定義のまま、cancel() を足します）。
enum OrderStatus: string
{
case Pending = 'pending';
case Confirmed = 'confirmed';
case Cancelled = 'cancelled';
public function canBeConfirmed(): bool
{
return $this === self::Pending;
}
public function canBeCancelled(): bool
{
return $this === self::Pending;
}
}
final class Order
{
// コンストラクタ、place()、reconstitute()、confirm() は既存の定義のまま（省略）
public function cancel(): void
{
if (! $this->status->canBeCancelled()) {
throw new OrderCannotBeCancelled($this->id, $this->status);
}
$this->status = OrderStatus::Cancelled;
}
}
final class CancelOrder
{
public function __construct(
private OrderRepository $orders,
private TransactionManager $transaction,
) {
}
public function execute(OrderId $id): void
{
$this->transaction->run(function () use ($id): void {
$order = $this->orders->findOrFail($id);
$order->cancel();
$this->orders->save($order);
});
}
}
OrderStatus は例として3つの状態だけにしています。キャンセルした日時や操作した人を記録する要件があれば、confirm() と同じく Order に持たせます。
「他人の注文はキャンセルできない」は、状態のルールではなく認可なので、Order ではなく UseCase を呼ぶ前に確かめます。ここでは、Controller が受け取る FormRequest の authorize() で Policy を呼ぶ前提にします（false を返すと 403 になります）。
final class CancelOrderRequest extends FormRequest
{
public function authorize(): bool
{
$order = OrderModel::findOrFail($this->route('orderId'));
return $this->user()->can('cancel', $order); // OrderPolicy::cancel() で注文の所有者と照合する
}
}
public function test_他人の注文はキャンセルできない(): void
{
$order = OrderModel::factory()->pending()->create();
$other = User::factory()->create();
$this->actingAs($other)
->postJson("/api/orders/{$order->id}/cancel")
->assertForbidden();
$this->assertSame('pending', $order->fresh()->status);
}
認可の置き場所が Policy か UseCase かは、既存のコードがどちらで確かめているかに合わせます。AI のコードで抜けやすいのは、置き場所の選び方より、確かめること自体です。
レビューでは、「この処理と同じ種類のことをしている既存のコードはどこか」を先に思い浮かべ、それと形を比べます。AI は手元のファイルから既存のパターンを読み取ることもあれば、一般的な Laravel の書き方で書くこともあります。どちらになったかは、比べないと分かりません。
4. 失敗したときに、何が起きるか
最後に、例外の扱いを見ます。生成されたコードには、止まらずに動き続けることを優先した書き方が混ざりやすいと感じています。
catch (Throwable $e)でログだけ出して処理を続ける- 取得に失敗したら
nullや空配列を返し、呼び出し側では区別できない - 外部 API の呼び出しに、条件を区別しないリトライが付いている
どれも、正常系のテストでは見えません。外部 API なら「再送してよい失敗か」、DB なら「途中まで更新された状態が残らないか」を、この段階で確かめます。
なぜそう判断したか
1行ずつ全部読む方法を選ばなかった理由
人のコードのレビューでは、要件・責務・境界条件などの観点を、差分を読みながら確かめます。AI のコードでも観点は同じですが、差分の量が多く、しかも全体が均一に整っているので、上から順に読むと注意が均等に薄まります。
そこで、読む前に範囲を絞る（1）、何を確かめるべきかをテストで先に知る（2）という順にしています。範囲とテストを確かめた後なら、実装を読むときに「どこを疑えばよいか」が分かっています。
テストが通ることを根拠にしない理由
AI が書いたテストは、AI が Spec をどう解釈したかを表しています。実装とテストが同じ解釈から書かれているので、解釈が間違っていれば、両方が同じように間違えたまま通ります。テストが通ることは「実装とテストが一致している」ことの証拠にはなりますが、「要件を満たしている」ことの証拠にはなりません。要件との対応を確かめられるのは、Spec を決めた人だけです。
AI のレビューで済ませなかった理由
私も AI にレビューさせています。見落としの指摘には役立ちます。ただ、AI のレビューが見るのは主に差分とその周辺で、「Spec で何を決めたか」「この設計にした理由は何か」は、書かれていなければ分かりません。受け入れ条件との対応と、既存の設計との整合は、人が確かめる部分として残しています。
どこから設計過剰になるか
- typo の修正や依存パッケージの更新のように、範囲が明らかで設計判断を含まない変更なら、差分の範囲（1）と CI の結果を確かめれば十分です
- 捨てる前提の PoC なら、既存の設計との整合（3）はほとんど問いません。確かめたい失敗条件がテストか手動の確認で見えれば足ります
- 同じ指摘を何度もしているなら、レビューで繰り返すより機械に移します。「UseCase から Eloquent や Facade を使わない」のような依存の規則は、静的解析のルールやアーキテクチャのテストで CI に確かめさせられます。レビューで人が見るのは、機械では表せない「この Spec にこの実装で合っているか」に絞ります
- AI のコードに、人が書いたコードより厳しい基準を当てる必要はありません。基準は同じで、変えるのは確かめる順序です
まとめ
- AI のコードは全体が整っているので、見た目の品質はレビューの手がかりにならない
- 差分の範囲を Plan と比べ、テストを受け入れ条件と対応させてから、既存の設計との整合と失敗時の振る舞いを見る
- テストが通ることは要件を満たすことの証拠にならない。繰り返す指摘は CI に移し、人は要件との対応に集中する