pr-review-assistant

PR レビューアシスタントスキル

目的

構文やスタイルの確認にとどまらず、amplihack の開発の中核となる原則に沿っているかを確認する、理念を重視したプルリクエストレビューです。このスキルでは、正しさだけでなく、徹底したシンプルさ、モジュール型アーキテクチャ、そしてごまかしのない実装という観点から PR をレビューします。

このスキルを使用する場面

• PR コードレビュー：amplihack の理念に関する原則に照らして PR をレビューします
• 理念への準拠：コードが徹底したシンプルさとブリックモジュール設計を体現しているか確認します
• リファクタリングの提案：過剰なエンジニアリングを特定し、具体的な簡素化案を提示します
• アーキテクチャの検証：モジュール設計と明確な契約を検証します
• テストカバレッジ：変更された機能に対して、テストが十分か評価します
• 設計の評価：マージ前に過剰なエンジニアリングを見つけます

中核となる理念：レビューの観点

1. 徹底したシンプルさ

コードのすべての行には、存在する理由が必要です。次の点を確認します。

• もっとシンプルにできませんか？ 各関数は、それぞれの役割を適切に果たしていますか？
• 今これが必要ですか？ それとも将来への備えですか？
• 不要な抽象化はありませんか？ 価値を付加しない余分なレイヤーはありませんか？
• 行数を減らせませんか？ 最良のコードとは、存在しないコードです。

2. モジュール型アーキテクチャ（ブリック＆スタッズ）

コードは、明確な接続部を備えた自己完結型のモジュールとして構成する必要があります。

• ブリック＝明確な責務を1つだけ持つ自己完結型のモジュール
• スタッズ＝他の要素が接続するための公開契約（関数、API、データモデル）
• 再生成可能＝接続を壊すことなく、仕様から再構築できること

3. ごまかしのない実装

近道、スタブ、技術的負債は認めません。

• コード内に TODO を残さない＝実際に実装するか、含めないこと
• NotImplementedError を使わない＝抽象基底クラスは例外です
• モックデータを使わない＝最初から実際に機能するものを実装すること
• デッドコードを残さない＝未使用のコードは削除すること
• すべての関数が機能すること＝機能しない関数は存在させないこと

4. スピードより品質

• 堅牢な実装＝応急処置より優れています
• 長期的な保守性＝短期的な利点より重視します
• 明確なエラー処理＝エラーを隠さず、見えるようにします
• テスト済みの動作＝モジュール境界で契約を検証します

レビュープロセス

ステップ1：変更内容を理解する

まず、PR で何が変更されたのかを理解します。

1. PR の説明を読む：変更の意図を理解します
2. 影響を受けるモジュールを特定する：対象範囲を確認します
3. 変更または追加された依存関係を記録する
4. 解決しようとしている問題を理解する

ステップ2：理念との整合性を確認する

変更点を1つずつ、amplihack の原則に照らしてレビューします。

徹底したシンプルさの確認

• すべての行が必要ですか？
• 不要な抽象化はありませんか？
• もっとシンプルに実装できませんか？
• 将来への備えや推測に基づく実装はありませんか？
• 重複した関数や似た関数はありませんか？
• 条件分岐を簡潔にできませんか？

モジュール構造の確認

• 変更はモジュール境界を尊重していますか？
• 公開契約は明確で、文書化されていますか？
• 内部ユーティリティは分離されていますか？
• モジュールの責務は明確に1つだけですか？
• 循環依存はありませんか？

ごまかしのない実装の確認

• TODO や NotImplementedError の呼び出しはありませんか？
• モックデータやテストデータが本番コードに公開されていませんか？
• エラー処理は明示的で、見えるようになっていますか？
• すべての関数が実際に動作する実装になっていますか？
• デッドコードや未使用の変数はありませんか？

ステップ3：過剰なエンジニアリングを特定する

よくある過剰なエンジニアリングのパターンを探します。

• 過剰な抽象化：明確な利点のない基底クラス、プロトコル、ファクトリー
• 汎用的な「フレームワーク」：仮定上のニーズに備えるためのインフラ構築
• 早すぎる最適化：重要ではない処理経路に複雑なアルゴリズムを導入すること
• 設定の複雑化：5行のデフォルト設定で済むところに、50行の設定を導入すること
• 将来への備え：「いつか必要になるかもしれない」コード
• 過剰な階層化：必要以上に間接的な構造
• 過剰なパラメーター化：よりシンプルな方法があるのに、8個以上のパラメーターを持つ関数

ステップ 4：Brick モジュールの構造を検証する

新しいモジュールを追加する場合、またはモジュールに変更を加える場合：

• 単一責任ですか？ このモジュールが行うことは何ですか？
• 公開インターフェースは明確ですか？ 何がエクスポートされ、その理由は何ですか？
• 内部は分離されていますか？ ユーティリティはモジュール内に収まっていますか？
• 依存関係は文書化されていますか？ 何に依存していますか？
• テストは含まれていますか？ 仕様でテスト要件が定義されていますか？
• 例は提供されていますか？ 使用方法は明確ですか？
• 再生成可能ですか？ 仕様から再構築できますか？

ステップ 5：テストカバレッジを確認する

適切なテストは非常に重要です。

• 契約の検証：テストで公開インターフェースの動作を検証している
• エッジケースを網羅：null、空の値、境界条件をテストしている
• エラーパスをテスト：想定どおりに例外が発生することをテストしている
• 統合テスト：モジュール間の接続を検証している
• 十分なカバレッジ：変更したコードのカバレッジが 85% 以上

ステップ 6：建設的なフィードバックを提供する

変更を提案する際は：

1. 具体的にする：ファイル名と行番号を示す
2. 理由を説明する：どの原則に反しているのかを伝える
3. 方法を提案する：具体例を示す
4. 敬意を払う：人ではなくコードに焦点を当てる
5. 良い仕事を認める：うまくできている点を評価する

具体的なレビュー用チェックリスト

徹底したシンプルさ

• すべての関数に、明確で単一の目的がある
• 不要な抽象化レイヤーがない
• 将来への過剰な備えや憶測がない
• ロジックや関数の重複がない
• 条件分岐がわかりやすい
• 変数名が明確で、自己説明的である
• 関数の引数が過剰でない

モジュール型アーキテクチャ

• モジュールに明確で単一の責任がある
• 公開インターフェースが最小限かつ明確である
• 内部ユーティリティが適切に分離されている
• 依存関係が明示されている
• 循環依存がない
• 境界での契約が明確である
• モジュール単体で理解できる

ごまかしのない実装

• TODO、NotImplementedError、スタブがない
• 本番コードにモックやテストデータがない
• デッドコードや未使用のインポートがない
• エラー処理が明示され、見える形になっている
• すべての関数に動作する実装がある
• 例外を握りつぶしていない
• デバッグ用のログやエラーメッセージが明確である

テストカバレッジ

• 公開インターフェースがテストされている
• エッジケースを網羅している
• エラー条件がテストされている
• 統合箇所が検証されている
• 十分なカバレッジがある（85% 以上）
• テストが実装ではなく契約を検証している

ドキュメント

• Docstring が明確で、必要な情報を網羅している
• 公開インターフェースが文書化されている
• 新機能の例が提供されている
• 必要に応じてモジュールの README が更新されている
• 型ヒントがあり、正確である

レビュー例

例 1：過剰な設計の特定

PR：API にユーザー権限チェックを追加

変更されたコード：

class PermissionValidator:
    def __init__(self):
        self.cache = {}

    def validate(self, user, resource):
        if user in self.cache:
            return self.cache[user]

        result = self._complex_validation(user, resource)
        self.cache[user] = result
        return result

    def _complex_validation(self, user, resource):
        # Complex business logic...
        pass

レビューコメント：

FILE: permissions.py (lines 10-25)

This over-engineers the permission checking with caching that may not be needed.
The caching layer adds complexity without proven benefit:

1. Cache can become stale if user permissions change
2. Unclear when/if cache should be invalidated
3. In-memory cache doesn't scale across processes
4. Permission checks are usually not in hot paths

SUGGESTION - Start simpler:
```python
def check_permission(user, resource):
    """Check if user can access resource."""
    # Direct implementation
    return user.has_access_to(resource)

キャッシュが後で必要になった場合は、プロファイリングによって効果があると確認できてから追加してください。

これは徹底したシンプルさの考え方に沿っています。必要性が証明されるまでは、複雑さを加えないでください。

### Example 2: Identifying Lack of Regeneration Documentation

**PR**: Add new authentication module

**Code Changed**: New file `~/.amplihack/.claude/tools/auth/auth.py`

**Review Comment**:

FILE: .claude/tools/auth/ （新しいモジュール）

良い点：モジュールの責任が明確です（JWT の検証、トークンの更新）。 問題点：モジュール再生成のための仕様ドキュメントがありません。

Brick 哲学の要件：モジュールは仕様から再生成できる必要があります。

必要な対応：

1. 次の内容を含む Specs/authentication.md を作成してください。
   • 明確な目的と範囲
   • 公開インターフェース（エクスポートされる関数）
   • 依存関係の一覧
   • テスト要件
   • 使用例

これにより、要件が変わった場合に再構築できます。

仕様テンプレートについては、.claude/skills/pr-review-assistant/ を参照してください。

### Example 3: Identifying Zero-BS Issues

**PR**: Add data processing pipeline

**Code Changed**:
```python
def process_data(data):
    """Process data through pipeline."""
    # TODO: Add error handling
    result = validate(data)
    if not result:
        return None  # swallowed error

    # TODO: Implement caching
    return transform(result)

レビューコメント：

FILE: pipeline.py (lines 5-15)

Zero-BS Issues Found:

1. LINE 6: TODO comments - implement or remove
   - "Add error handling" - Either add it now or create issue
   - "Implement caching" - Not needed yet; add if profiling shows benefit

2. LINE 10: Swallowed error
   - validate() likely raised exception, but returns None
   - Make error explicit: raise ValueError("Invalid data")
   - Or catch and re-raise with context

3. LINE 8: Silent failures
   - If None is returned, caller doesn't know why
   - Either raise exception or return Result type

SUGGESTION:
```python
def process_data(data):
    """Process data through pipeline.

    Raises:
        ValueError: If data validation fails
    """
    result = validate(data)  # Raises if invalid
    return transform(result)

すべてのコードは本番環境に対応しており、TODO はありません。

### Example 4: Identifying Missing Tests

**PR**: Add user role management

**Code Changed**: New class `RoleManager` in user module

**Review Comment**:

FILE: user.py （50～100 行目）

新しい機能が追加されていますが、十分なテストがありません。

問題点：RoleManager の公開インターフェースがテストされていません。

• add_role(user, role) - 成功ケースを追加
• remove_role(user, role) - 成功ケースを追加
• get_roles(user) - 空、単一、複数のケースをテスト
• is_admin(user) - true/false のケースをテスト

不足しているエッジケース：

• 重複したロールの割り当て（冪等であること）
• 存在しないロールの削除（エラーにならないこと）
• ロールがないユーザーに対する get_roles（空を返すこと）

コントラクトの検証：

• 関数がドキュメントに記載された型を受け付ける
• 戻り値の型がドキュメントと一致する
• ドキュメントに記載された例外が発生する

追加：カバレッジ 85% 以上の tests/test_role_manager.py

### Example 5: Recognizing Good Design

**PR**: Add simplified config loader

**Code Changed**:
```python
def load_config(path):
    """Load YAML config from file.

    Args:
        path: Path to config file

    Returns:
        dict: Parsed config

    Raises:
        FileNotFoundError: If file doesn't exist
        ValueError: If YAML is invalid
    """
    with open(path) as f:
        try:
            return yaml.safe_load(f)
        except yaml.YAMLError as e:
            raise ValueError(f"Invalid YAML: {e}") from e

レビューコメント：

EXCELLENT - This is exactly the simplicity we want:

✅ Single responsibility: Load and parse YAML
✅ Clear error handling: Explicit exceptions with context
✅ No TODOs or stubs: Production ready
✅ Complete documentation: Clear args, returns, exceptions
✅ No over-engineering: Direct implementation
✅ Testable: Clear behavior to verify

This is a model example of ruthless simplicity.

フィードバックテンプレート

PR にコメントするときは、次の構成を使用してください。

**FILE**: path/to/file.py (lines X-Y)

**ISSUE**: [Principle violated - Simplicity/Modularity/Zero-BS/Tests/Docs]

**WHAT**: [Describe what's in the code]

**WHY IT'S PROBLEMATIC**: [How it violates amplihack principles]

**SUGGESTION**: [Concrete code example or approach]

**REFERENCE**: [Link to relevant philosophy, principle, or example]

GitHub との連携

レビューコメントの投稿

このスキルでは、次の方法で GitHub の PR にレビューコメントを投稿できます。

gh pr comment <PR-NUMBER> -b "Review comment here"
# Or for specific file reviews:
gh pr diff <PR-NUMBER> | grep "^---" | head -1
# Then post review with specific file:line references

レビューワークフロー

1. PR の詳細を取得：PR 番号、ブランチ、変更されたファイルを取得します
2. 変更を分析：哲学に照らしてレビューを実行します
3. フィードバックを生成：具体的で実行可能なコメントをまとめます
4. レビューを投稿：すべてのコメントを含む GitHub のレビューを作成します
5. 概要：全体的な評価を投稿します

よくある過剰設計パターン

パターン 1：設定の複雑化

# OVER-ENGINEERED: 50-line config class
class ConfigManager:
    def __init__(self, env_file, schema_file, validators):
        self.config = load_yaml(env_file)
        self.schema = load_json(schema_file)
        self.validators = validators
        # 40 more lines...

# SIMPLE: 5 lines
config = yaml.safe_load(open('.env.yaml'))

パターン 2：不要なファクトリーパターン

# OVER-ENGINEERED: Factory for single implementation
class ValidationFactory:
    def create_validator(self, type):
        if type == "email":
            return EmailValidator()
        # ... more types

# SIMPLE: Direct function
def validate_email(email):
    return "@" in email and "." in email

パターン 3：用途が 1 つだけの汎用基底クラス

# OVER-ENGINEERED: Base class never subclassed
class BaseRepository(ABC):
    @abstractmethod
    def find(self, id): pass
    # ... 20 abstract methods

class UserRepository(BaseRepository):
    # Forced to implement all abstract methods
    # But only uses 3 of them

# SIMPLE: Direct class
class UserRepository:
    def find(self, id):
        return self.db.query(User).get(id)

パターン 4：時期尚早な最適化

# OVER-ENGINEERED: Complex caching for cache that's not needed
cache = LRUCache(maxsize=1000)
stats = CacheStats()
lock = threading.Lock()
# ... complex logic

# SIMPLE: None - profile first, optimize if needed
result = function(args)

確認すべき主な質問

レビュー時には、次の質問をしてください。

1. もっとシンプルにできますか？ できるなら、なぜそうなっていないのでしょうか？
2. 今これが必要ですか？ それとも将来への備えでしょうか？
3. これが行うことは 1 つだけですか？ 複数ある場合は、分割してください。
4. 誰がこれを使いますか？ その人にとってインターフェースは明確ですか？
5. 何が問題になり得ますか？ エラーは明示的に処理されていますか？
6. テスト可能ですか？ コントラクトを検証できますか？
7. これは変更が必要になりますか？ 過剰な設計をせずに柔軟性を持たせていますか？
8. これは削除できますか？ リファクタリングできるかどうかよりも、こちらを考えてください。
9. これはチームのパターンに従っていますか？ それとも独自のものですか？
10. これが動作すると確信できますか？ それとも推測に基づいていますか？

成功基準

このスキルを使った PR レビューが成功している状態：

• スタイルだけでなく、amplihack の哲学に照らしてコードをレビューしている
• 具体的な提案とともに過剰設計を特定している
• モジュール構造とブリックの設計を検証している
• テストカバレッジが十分か確認している
• ファイル名と行番号を具体的に示している
• 具体的で実行可能な提案をしている
• 優れた設計を認識し、評価している
• 包括的な GitHub レビューコメントを投稿している
• チームの学習と改善に役立っている

出力

このスキルは、以下を生成します。

1. 理念準拠レポート

   • 徹底したシンプルさのチェック
   • モジュール型アーキテクチャのチェック
   • 率直な実装のチェック
   • テストカバレッジの評価
   • 総合評価

2. 具体的な推奨事項

   • 過剰な設計の特定と例
   • コード付きの簡素化の提案
   • モジュール構造に関するフィードバック
   • 対処すべきテストの不足

3. GitHub コメント（任意）

   • ファイル名と行番号を参照した詳細なレビュー
   • インラインのコード提案
   • 所見の要約
   • 建設的で敬意のある語調

理念に関する参照資料

すべてのレビューは、以下のドキュメントを基盤とします。

• ~/.amplihack/.claude/context/PHILOSOPHY.md - 開発理念の中核
• ~/.amplihack/.claude/context/PATTERNS.md - 承認済みパターンとアンチパターン
• Specs/ - アーキテクチャ検証用のモジュール仕様
• ~/.amplihack/.claude/context/DISCOVERIES.md - 既知の問題と解決策

効果的なレビューのヒント

1. 具体的にする：正確な行とコードを参照する
2. 理由を説明する：どの原則に違反しているか、なぜ重要なのかを示す
3. 方法を提案する：具体的なコード例を示す
4. 制約を尊重する：複雑さが必要な場合もあります
5. 良い点を認める：うまくできている点を称賛する
6. 質問する：「検討したことはありますか？」と問いかけて議論を促す
7. 共に学ぶ：レビューは学びの機会です
8. 反復する：完璧さを求めず、改善を提案する
9. 背景を考慮する：外部の制約も重要です
10. 焦点を絞る：個人のスタイルではなく、理念との整合性をレビューする

関連するスキルとワークフロー

• モジュール仕様ジェネレーター：再生成可能なモジュールの仕様を作成します
• ビルダーエージェント：仕様に基づいてコードを実装します
• レビュアーエージェント：理念への準拠を検証します
• テスターエージェント：テストを生成し、検証します
• ドキュメント駆動開発：仕様を信頼できる唯一の情報源として使用します

フィードバックと進化

このスキルは、利用状況に応じて進化させる必要があります。

• どのようなパターンが繰り返し見つかっていますか？
• どのような提案が、より良いコードにつながっていますか？
• どの理念が最も頻繁に守られていませんか？
• 問題をより早く発見するにはどうすればよいですか？

学びを ~/.amplihack/.claude/context/DISCOVERIES.md に記録してください。