rails-code-review

Rails 代码审查（Rails 之道）

审查 Rails 代码时，请根据以下方面进行分析。编写新代码时，请遵循 rails-code-conventions（原则、日志记录、路径规则）和 rails-stack-conventions（特定技术栈的 UI 和 Rails 模式）。

核心原则： 尽早审查，频繁审查。提交 PR 前先自我审查。重大改动后重新审查。

预检

开始审查前，请确认以下事项：

• 已检测到 Rails 项目：确认当前目录属于 Rails 应用（检查 config/application.rb、包含 rails 的 Gemfile 或 bin/rails）
• 差异可用：确保可以访问代码变更（git diff、PR 差异或文件路径）
• 文件存在：确认差异中引用的所有文件都存在于仓库中
• 审查范围明确：确认审查的是完整 PR、特定功能还是指定文件

如果预检未通过：

• 不是 Rails 项目 → 使用适用于相应技术栈的审查技能
• 没有可用的差异 → 请用户提供差异或文件路径
• 文件缺失 → 标记为 Critical，并在继续前请求澄清

硬性关卡：实现完成后（提交 PR 前）

After green tests + linters pass + YARD + doc updates:
1. Self-review the full branch diff using the Review Order below.
2. Fix Critical items; resolve or ticket Suggestion items.
3. Only then open the PR.
generate-tasks must include a "Code review before merge" task.

快速参考

领域	重点检查项
路由	RESTful、浅层嵌套、命名路由、约束
控制器	保持精简、强参数、before_action 作用域
模型	结构顺序、inverse_of、枚举值、优先使用作用域而非回调
查询	防止 N+1、优先用 exists? 而非 present?、批量处理使用 find_each
迁移	可逆、已建立索引、外键、并发索引
安全	强参数、参数化查询、不滥用 html_safe
缓存	片段缓存、嵌套缓存、ETag
作业	幂等、可重试、使用合适的后端

审查顺序

按以下顺序检查差异。详细标准见：REVIEW_CHECKLIST.md。PR 单页基准清单见：assets/checklist.md。问题示例（JSON 和评论格式）见：assets/examples.md。

配置 → 路由 → 控制器 → 视图 → 模型 → 关联 → 查询 → 迁移 → 验证 → I18n → 会话 → 安全 → 缓存 → 作业 → 测试

边缘情况处理：

• 差异为空：如果没有文件变更，请说明“没有需要审查的代码变更”，然后直接进入结论
• 差异很大（超过 50 个文件）：优先检查 Critical 项，然后抽查关键文件中的 Suggestion 项；标记并安排有针对性的后续审查
• 仅修改单个文件：对该文件应用所有相关审查领域；不要因为差异较小而跳过任何领域
• 仅修改测试：重点关注测试质量、覆盖率和测试组织；跳过应用代码检查

立即识别的关键检查项：

# N+1 — one query per record in a collection
posts.each { |post| post.author.name }       # Bad
posts.includes(:author).each { |post| post.author.name }  # Good

# Privilege escalation via permit!
params.require(:user).permit!                # Bad — never in production
params.require(:user).permit(:name, :email)  # Good

始终标记为 Critical（每次出现都标记为 Critical）：

• params.require(...).permit! — 批量赋值／权限提升
• 对用户提供的内容使用 html_safe 或 raw — XSS
• 敏感操作缺少授权检查
• 控制器操作中包含业务逻辑 — 价格、税费、折扣、多步骤工作流或任何内联的领域计算。控制器操作只要不止于协调（调用一个服务、渲染响应），就属于 Critical，而非 Suggestion。
• 未参数化／使用字符串插值的 SQL — 注入
• 针对大型表且没有安全方案的破坏性迁移

严重级别

只能使用以下标签（不得使用 High/Low、P0–P2 等）：Critical | Suggestion | Nice to have。

• Critical — 安全问题、数据丢失、崩溃，或任何始终标记为 Critical的规则 → 阻止合并；修复后重新检查差异。
• Suggestion — 规范／性能问题 → 在 PR 中修复；如果需要大规模重新设计，则创建工单。
• Nice to have — 小型风格改进或微优化 → 作者可自行决定是否处理。

输出风格

将发现按 ### Critical / ### Suggestion / ### Nice to have 分组（省略空章节）。不要将不同严重程度的发现混在一个平铺列表中。

## Review — <PR title or area>

### Critical
- [path/to/file.rb:LINE] (Area) One-line risk. **Mitigation:** concrete next step.

### Suggestion
- [path/to/file.rb:LINE] (Area) … **Mitigation:** …

### Nice to have
- …

**Actions required:** <one line per severity level that appeared — e.g. Critical → block merge + re-review; Suggestion → …>

**模板规则：**每个项目符号采用 [file:line] (Area) + 风险 + Mitigation:（必填）。从以下类别中标注 (Area)：Controllers、Routing、Views、Models、Queries、Migrations、Validations、Security、Caching、Jobs、Tests — 如果差异涉及多个方面，整个评审应覆盖至少 4 个不同类别。

输出验证：

• 确认 [file:line] 引用中的所有文件路径都存在于代码仓库中
• 确保行号在各文件的有效范围内
• 检查每项发现是否都包含必填的 Mitigation: 字段
• 确认严重程度章节分类正确
• 验证 Actions required: 章节准确总结了所有发现

如果文件引用无效：

• 跳过该发现并注明：[path/to/file.rb:LINE] — File not found in repository, skipping
• 在最终评审中包含该项之前，请用户澄清

合并前重新评审

任何 Critical 修复后必须重新比对分支差异；反馈期间修复超过 3 项 Suggestion，或发生任何逻辑／架构变更后，建议重新比对；修复可能影响查询、身份验证或迁移时，也应重新比对。只有仅涉及 Nice to have 的反馈，或不改变行为的简单单行编辑，才可跳过。

评审反模式（补充检查清单，但不替代它）

• 精简控制器 → 臃肿模型：将编排逻辑提取到服务（PORO / *.call）中，而不是写成庞大的模型方法。
• **开发环境中的 N+1：**少量种子数据会掩盖 N+1 问题 — 如果关联数据在循环中加载，应通过查询次数（request spec、rack-mini-profiler、日志）进行统计，而不是想当然地认为“这里运行很快”。
• **热表迁移：**将并发索引和大量回填安排在与可逆 schema 变更不同的部署步骤中（不确定时，串联 rails-migration-safety）。
• **回调与任务：**持久化钩子仅用于持久化；外部 I/O 和多步骤工作流应放在服务／任务中，并明确幂等性。

集成

技能	串联时机
rails-review-response	开发者收到反馈并需要决定实施哪些内容时
rails-architecture-review	评审发现结构性问题时
rails-security-review	评审发现安全问题时
rails-migration-safety	评审大型表的迁移时
refactor-safely	评审建议进行重构时