pr-review-assistant

PR 审查助手技能

目的

以理念为导向的拉取请求审查，不止检查语法和风格，还会检查是否符合 amplihack 的核心开发原则。此技能审查 PR 时，不只关注正确性，还会严格检查是否做到极致简洁、模块化架构和零废话实现。

何时使用此技能

• PR 代码审查：依据 amplihack 理念原则审查 PR
• 理念合规性：检查代码是否体现了极致简洁和积木模块设计
• 重构建议：找出过度工程化，并提出具体的简化建议
• 架构验证：验证模块化设计和清晰的契约
• 测试覆盖率：评估变更功能的测试是否充分
• 设计评估：在合并前发现过度工程化

核心理念：审查重点

1. 极致简洁

每一行代码都必须证明其存在的价值。我们会问：

• 能否更简单？ 每个函数是否都能出色地完成一件事？
• 现在是否需要？ 还是在为未来做准备？
• 是否存在不必要的抽象？ 是否增加了没有价值的额外层次？
• 能否删掉一些代码行？ 最好的代码，就是不存在的代码。

2. 模块化架构（积木与连接件）

代码应组织为自包含的模块，并通过清晰的连接方式组合：

• 积木 = 职责明确且自包含的模块
• 连接件 = 公共契约（函数、API、数据模型），供其他模块连接
• 可再生成 = 可根据规范重新构建，且不会破坏连接

3. 零废话实现

不走捷径、不留存根、不留技术债务：

• 代码中不留 TODO = 要么真正实现，要么不要包含
• 不使用 NotImplementedError = 抽象基类中除外
• 不使用模拟数据 = 从一开始就提供真实功能
• 不留死代码 = 删除未使用的代码
• 每个函数都能运行 = 否则就不应存在

4. 质量优先于速度

• 稳健的实现 = 优于快速修补
• 长期可维护性 = 不追求短期收益
• 清晰的错误处理 = 错误应明确可见，不应被吞掉
• 经过测试的行为 = 在模块边界验证契约

审查流程

第 1 步：了解变更

首先了解 PR 做了哪些变更：

1. 阅读 PR 描述，了解变更意图
2. 确定受影响的模块及其范围
3. 记录变更或新增的依赖项
4. 了解要解决的问题

第 2 步：检查理念契合度

依据 amplihack 原则审查每项变更：

极致简洁检查

• 每一行代码都必要吗？
• 是否存在不必要的抽象？
• 能否用更简单的方式实现？
• 是否存在为未来预做准备或臆测需求的情况？
• 是否存在重复或相似的函数？
• 能否简化条件逻辑？

模块结构检查

• 变更是否遵守模块边界？
• 公共契约是否清晰且有文档说明？
• 内部工具函数是否彼此隔离？
• 模块是否只有一个明确职责？
• 是否存在循环依赖？

零废话检查

• 是否有 TODO 或 NotImplementedError 调用？
• 生产代码中是否暴露了模拟数据或测试数据？
• 错误处理是否明确且可见？
• 所有函数是否都已实现并可运行？
• 是否存在死代码或未使用的变量？

第 3 步：识别过度工程化

留意常见的过度工程化模式：

• 过度抽象：没有明确收益却引入基类、协议或工厂
• 通用型“框架”：为假设的需求搭建基础设施
• 过早优化：为非关键路径使用复杂算法
• 配置复杂化：本可用 5 行默认配置解决，却写了 50 行配置
• 面向未来的预备代码：“以后可能会需要”之类的代码
• 层次过多：间接层次超过必要程度
• 参数过多：函数有 8 个或更多参数，而非采用更简单的方案

步骤 4：验证 Brick 模块结构

如果有新增模块或模块变更：

• 职责单一？ 这个模块做的唯一一件事是什么？
• 公共接口清晰？ 导出了什么，为什么要导出？
• 内部隔离？ 工具函数是否限制在模块内部？
• 依赖有记录？ 它依赖什么？
• 包含测试？ 规范是否定义了测试要求？
• 提供示例？ 用法是否清楚？
• 可重新生成？ 能否根据规范重新构建？

步骤 5：检查测试覆盖率

充分的测试至关重要：

• 契约验证：测试验证公共接口行为
• 覆盖边界情况：测试空值、空内容和边界条件
• 测试错误路径：在预期情况下引发异常
• 测试集成：验证模块间的连接
• 覆盖率充足：变更代码的覆盖率达到 85% 以上

步骤 6：提供建设性反馈

提出变更建议时：

1. 具体明确：引用文件和行号
2. 解释原因：违反了什么原则？
3. 建议做法：提供具体示例
4. 保持尊重：关注代码，而非个人
5. 肯定出色工作：认可做得好的地方

具体审查清单

极致简洁

• 每个函数都有一个明确的用途
• 没有不必要的抽象层
• 没有为未来做过度设计或猜测
• 没有重复的逻辑或函数
• 条件逻辑简单明了
• 变量名清晰且能自我说明
• 函数签名没有过多参数

模块化架构

• 模块只有一个明确的职责
• 公共接口精简且清晰
• 内部工具函数已妥善隔离
• 依赖关系明确
• 没有循环依赖
• 边界处的契约清晰
• 模块可以独立理解

零废话实现

• 没有 TODO、NotImplementedError 或占位代码
• 生产代码中没有模拟数据或测试数据
• 没有死代码或未使用的导入
• 错误处理明确且可见
• 所有函数都有可运行的实现
• 没有吞掉异常
• 日志和错误消息清晰，便于调试

测试覆盖率

• 已测试公共接口
• 已覆盖边界情况
• 已测试错误条件
• 已验证集成点
• 覆盖率充足（85% 以上）
• 测试验证契约，而非实现细节

文档

• 文档字符串清晰完整
• 已记录公共接口
• 为新功能提供示例
• 必要时更新模块 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**:

文件：.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**:

文件：user.py（第 50-100 行）

新增功能缺少充分的测试。

问题：未测试 RoleManager 公共接口

• add_role(user, role) - 添加成功场景测试
• remove_role(user, role) - 添加成功场景测试
• get_roles(user) - 测试空、单个和多个角色的情况
• is_admin(user) - 测试 true/false 场景

缺少边界情况测试：

• 重复分配角色（应具备幂等性）
• 移除不存在的角色（不应报错）
• 对没有任何角色的用户调用 get_roles（应返回空结果）

契约验证：

• 函数接受文档中规定的类型
• 返回类型与文档一致
• 按文档说明抛出异常

新增：tests/test_role_manager.py，覆盖率达到 85% 以上

### 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：仅用于一次场景的通用基类

# 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. 这只做一件什么事？ 如果做很多事，就拆分它。
4. 谁会使用它？ 对他们来说，接口是否清晰？
5. 可能出什么问题？ 是否明确处理错误？
6. 它是否可测试？ 能否验证其契约？
7. 它以后需要改变吗？ 是否灵活，同时又没有过度设计？
8. 能不能直接删除？ 这比问能不能重构更好。
9. 它是否遵循我们的模式？ 还是独树一帜？
10. 我有把握确认它能正常工作吗？ 还是这只是推测？

成功标准

使用此技能进行成功的 PR 审查时，应达到以下标准：

• 根据 amplihack 理念审查代码，而不只是检查风格
• 通过具体建议识别过度设计
• 验证模块结构和 brick 设计
• 检查测试覆盖率是否充分
• 提供具体的文件:行号引用
• 提供具体且可执行的建议
• 认可并肯定优秀的设计
• 发布全面的 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. 聚焦重点：评审理念是否契合，而非个人风格

相关技能和工作流

• 模块规格生成器：为可重新生成的模块创建规格
• 构建器 Agent：根据规格实现代码
• 评审 Agent：验证理念合规性
• 测试 Agent：生成和验证测试
• 文档驱动开发：以规格作为事实来源

反馈与演进

该技能应根据使用情况不断演进：

• 我们反复发现了哪些模式？
• 哪些建议能带来更好的代码？
• 哪些理念原则最常被违反？
• 如何更早发现问题？

将经验记录在 ~/.amplihack/.claude/context/DISCOVERIES.md 中。