6.2 KiB
6.2 KiB
レビューポリシー
全レビュアーが共有する判断基準と行動原則を定義する。
原則
| 原則 | 基準 |
|---|---|
| 即座修正 | 軽微でも「次のタスク」にしない。今修正できる問題は今修正させる |
| 曖昧さ排除 | 「もう少し整理して」等の曖昧な指摘は禁止。ファイル・行・修正案を具体的に示す |
| ファクトチェック | 推測ではなく実コードを確認してから指摘する |
| 実践的修正案 | 理想論ではなく実装可能な対策を提示する |
| ボーイスカウト | 変更したファイルに問題があれば、タスクスコープ内で改善させる |
スコープ判定
| 状況 | 判定 | 対応 |
|---|---|---|
| 今回の変更で導入された問題 | ブロッキング | REJECT |
| 今回の変更により未使用になったコード(引数、import、変数、関数) | ブロッキング | REJECT(変更起因の問題) |
| 変更ファイル内の既存問題 | ブロッキング | REJECT(ボーイスカウトルール) |
| 変更モジュール内の構造的問題 | ブロッキング | スコープ内なら REJECT |
| 変更外ファイルの問題 | 非ブロッキング | 記録のみ(参考情報) |
| タスクスコープを大きく逸脱するリファクタリング | 非ブロッキング | 提案として記載 |
判定基準
REJECT(差し戻し)
以下のいずれかに該当する場合、例外なく REJECT する。
- テストがない新しい振る舞い
- バグ修正にリグレッションテストがない
any型の使用- フォールバック値の乱用(
?? 'unknown') - 説明コメント(What/How のコメント)
- 未使用コード(「念のため」のコード)
- オブジェクト/配列の直接変更
- エラーの握りつぶし(空の catch)
- TODO コメント(Issue化されていないもの)
- 3箇所以上の重複コード(DRY違反)
- 同じことをするメソッドの増殖(構成の違いで吸収すべき)
- 特定実装の汎用層への漏洩(汎用層に特定実装のインポート・分岐がある)
- 関連フィールドのクロスバリデーション欠如(意味的に結合した設定値の不変条件が未検証)
Warning(警告)
ブロッキングではないが改善を推奨する。
- エッジケース・境界値のテスト不足
- テストが実装の詳細に依存
- 関数/ファイルが複雑すぎる
- 命名が不明確
- TODO/FIXME の放置(Issue番号付きは許容)
- 理由なしの
@ts-ignore、eslint-disable
APPROVE(承認)
全ての REJECT 基準をクリアし、品質基準を満たしている場合に承認する。「条件付き承認」はしない。問題があれば差し戻す。
ファクトチェック
指摘する前に必ず事実を確認する。
| やるべきこと | やってはいけないこと |
|---|---|
| ファイルを開いて実コードを確認 | 「修正済みのはず」と思い込む |
| grep で呼び出し元・使用箇所を検索 | 記憶に基づいて指摘する |
| 型定義・スキーマを突合 | 推測でデッドコードと判断する |
| 生成ファイル(レポート等)とソースを区別 | 生成ファイルをソースコードとしてレビュー |
具体的な指摘の書き方
全ての指摘には以下を含める。
- どのファイルの何行目か
- 何が問題か
- どう修正すべきか
❌ 「構造を見直してください」
❌ 「もう少し整理してください」
❌ 「リファクタリングが必要です」
✅ 「src/auth/service.ts:45 — validateUser() が3箇所で重複。
共通関数に抽出してください」
ボーイスカウトルール
来たときよりも美しく。
対象
- 変更したファイル内の既存の問題(未使用コード、不適切な命名、壊れた抽象化)
- 変更したモジュール内の構造的な問題(責務の混在、不要な依存)
対象外
- 変更していないファイル(既存問題として記録のみ)
- タスクスコープを大きく逸脱するリファクタリング(提案として記載、非ブロッキング)
判定
| 状況 | 判定 |
|---|---|
| 変更ファイル内に明らかな問題がある | REJECT — 一緒に修正させる |
| 冗長な式(同値の短い書き方がある) | REJECT |
| 不要な分岐・条件(到達しない、または常に同じ結果) | REJECT |
| 数秒〜数分で修正可能な問題 | REJECT(「非ブロッキング」にしない) |
| 変更の結果として未使用になったコード(引数・import等) | REJECT — 変更起因であり「既存問題」ではない |
| 修正にリファクタリングが必要(スコープが大きい) | 記録のみ(技術的負債) |
既存コードの踏襲を理由にした問題の放置は認めない。既存コードが悪い場合、それに合わせるのではなく改善する。
判定ルール
- 変更対象ファイル内で検出した問題は、既存コードであっても全てブロッキング(REJECT対象)として扱う
- 「既存問題」「非ブロッキング」に分類してよいのは、変更対象外のファイルの問題のみ
- 「コード自体は以前から存在していた」は非ブロッキングの理由にならない。変更ファイル内にある以上、ボーイスカウトルールが適用される
- 問題が1件でもあればREJECT。「APPROVE + 警告」「APPROVE + 提案」は禁止
堂々巡りの検出
同じ種類の指摘が繰り返されている場合、修正指示の繰り返しではなくアプローチ自体を見直す。
同じ問題が繰り返されたら
- 同じ種類の問題が繰り返されていないか確認
- 繰り返されている場合、細かい修正指示ではなくアプローチ自体の代替案を提示
- REJECT する場合でも、「別のアプローチを検討すべき」という観点を含める
「もう一度修正して」と繰り返すより、立ち止まって別の道を示す。