cfdaily
173 lines
7.5 KiB
Markdown
173 lines
7.5 KiB
Markdown
---
|
||
name: code-review-quality
|
||
description: >-
|
||
代码评审质量纪律：评审闭环必须包含最终代码确认、枚举一致性检查、
|
||
设计-代码三层对照、评审者自我纠正。在发评审、收评审结果、做评审时触发。
|
||
---
|
||
|
||
# 代码评审质量
|
||
|
||
> 来源：从三国团队（庞统/司马懿/诸葛亮）~2GB 对话历史中蒸馏
|
||
> 卡片数：6（合并自批次1卡片5/12 + 批次2卡片1/2/3/5）
|
||
|
||
## 适用场景
|
||
|
||
- 发出代码评审请求时
|
||
- 收到评审结果需要处理时
|
||
- 自己作为评审者审查他人代码时
|
||
- 多步骤开发中方案评审 → 实现的闭环确认时
|
||
|
||
## 经验清单
|
||
|
||
### 1. 评审闭环必须包含最终代码确认（severity: high）
|
||
|
||
**场景**：多步骤开发中，方案评审通过后，实现阶段可能有微调。评审者只确认了方案而没看最终代码，Bug 被遗漏。
|
||
|
||
**正确做法**：
|
||
- **一键三连 = 改文档 → 改代码 → 司马懿确认最终代码片段**
|
||
- 每个方案落地后，必须把最终代码（不是方案描述）发给评审者确认
|
||
- 司马懿在评审中多次引用代码行号做交叉验证，这种精度只有看最终代码才能做到
|
||
|
||
**⚠️ 常见错误**：
|
||
- 实现后只说"已采纳建议"，没发最终代码让评审者确认
|
||
- "已采纳" ≠ "已验证"——采纳后的代码可能引入新问题
|
||
|
||
**关键细节**：
|
||
- 庞统在 M2 阶段让司马懿 review 了方案但没发最终代码，用户发现后指出：[pangtong/experience]
|
||
> "一键三连最终肯定是以司马懿 review 完你的最终成果物收尾的"
|
||
- 司马懿多次在代码评审中发现方案评审没暴露的 Bug（如 `_do_retry` 续杯失效）
|
||
- 续杯评审中，司马懿看到最终代码才发现 `on_complete=None` 导致幻觉门控丢失
|
||
|
||
> 来源：批次2卡片1（评审闭环，freq=5+）
|
||
|
||
---
|
||
|
||
### 2. 状态机枚举值一致性是高频 Bug 根源（severity: high）
|
||
|
||
**场景**：系统有多处引用任务/节点状态值，枚举定义与实际使用不一致会导致静默失败。
|
||
|
||
**正确做法**：
|
||
1. 状态枚举集中定义，所有引用点从枚举取值，硬编码字符串零容忍
|
||
2. 状态转换函数只保留一个入口（如 `_transition_status`），禁止分散在多个模块
|
||
3. 评审时重点检查：新代码中的状态值是否与枚举定义一致
|
||
|
||
**⚠️ 常见错误**：
|
||
- SQL 查询用 `'executing'` 但实际枚举值是 `'working'`，导致重启恢复遗漏 [pangtong/simayi-review]
|
||
- `failed→pending` 转换不清 `assignee`，与另一路径行为不一致
|
||
- `max_per_agent=2` vs 设计文档的 `1`，配置与设计不一致
|
||
|
||
**关键细节**：
|
||
- 司马懿在 counter v2.1 评审中发现配置值与设计文档不匹配
|
||
- auto-sync 可能静默修改配置文件，需要与设计文档对照
|
||
- P0-2 状态不一致导致任务永久卡住，P0-1 导致重启恢复失败
|
||
|
||
> 来源：批次2卡片2（枚举一致性，freq=3+）
|
||
|
||
---
|
||
|
||
### 3. 评审者自我纠正是高质量评审的标志（severity: high）
|
||
|
||
**场景**：评审者在审查代码时可能做出错误判断。高质量的处理不是掩盖，而是主动承认并修正。
|
||
|
||
**正确做法**：
|
||
- 庞统发现两处自己之前的判断有误，主动发邮件给司马懿修正：
|
||
> "修正 1: 问题4 recent_compact 不是死代码，是设计实现遗漏" [pangtong/mail]
|
||
- 司马懿回复："你说得对，我判断错了" [simayi/mail]
|
||
- 修正邮件明确标注"修正"而非静默修改，保留审计轨迹
|
||
|
||
**⚠️ 常见错误**：
|
||
- 不主动修正，错误判断被当作已确认结论传播
|
||
- 后续实现基于错误假设，导致更严重问题
|
||
|
||
**关键细节**：
|
||
- 评审者翻回设计文档交叉验证，是发现误判的有效手段
|
||
- 庞统和司马懿在同一次评审中都做了自我纠正——交叉验证机制在工作
|
||
- 评审者不是全知的，承认错误比坚持错误更有价值
|
||
|
||
> 来源：批次2卡片3（自我纠正，freq=3）
|
||
|
||
---
|
||
|
||
### 4. 设计文档-代码一致性审查不可省（severity: high）
|
||
|
||
**场景**：迭代开发中设计文档更新不及时，或代码实现偏离设计。需要 PRD→设计→代码的三层一致性审查。
|
||
|
||
**正确做法**：
|
||
1. 每次代码评审前，先对设计文档逐条检查实现覆盖度
|
||
2. 设计文档用明确编号（§B1/B2/B3）便于逐条追踪
|
||
3. 安排"背靠背一致性审查"：PRD→设计→代码全覆盖审计
|
||
4. 司马懿的核心职责就是做这种三层对照
|
||
|
||
**⚠️ 常见错误**：
|
||
- 设计文档 §B2（compact 进行中继续等）在代码中完全遗漏
|
||
- 司马懿初评时也未发现，直到庞统重新读设计文档才暴露 [pangtong/mail]
|
||
> "代码只实现了 B1/B3/B4，B2 遗漏了。recent_compact 是 B2 实现的数据基础"
|
||
|
||
**关键细节**：
|
||
- 庞统专门发了 #311 邮件请求"PRD→设计→代码三层一致性审查"
|
||
- 司马懿在 counter 生命周期评审中发现设计说 crash 3 次标 failed，但代码只 release counter 无计数
|
||
- 三层审查的顺序：先 PRD→设计（需求是否覆盖），再设计→代码（实现是否忠实）
|
||
|
||
> 来源：批次2卡片4（设计-代码一致性，freq=4+）
|
||
|
||
---
|
||
|
||
### 5. 要求深度评审而非最小改动（severity: medium）
|
||
|
||
**场景**：将问题发给评审者时，评审标准应该是从需求和设计角度出发，而不是做最小改动。
|
||
|
||
**正确做法**：
|
||
1. 评审时明确要求从需求和设计角度出发
|
||
2. 不接受以"延后到后续版本"为由的需求降级
|
||
3. 评审要回答"这个方案是否满足原始需求"
|
||
4. 用户说"全部做"时，不接受延后建议
|
||
|
||
**⚠️ 常见错误**：
|
||
- 只要求评审者做"最小改动"或接受"需求降级"的建议
|
||
- 采纳评审者的"延后"建议，一步步推进
|
||
- 用户说"不要一步一步来"意味着要一次到位 [frag_2087]
|
||
|
||
**关键细节**：
|
||
- "需求降级"是 agent 常见的偷懒方式——把困难的需求推到下个版本
|
||
- "你知道的"暗示用户偏好一步到位，这是长期偏好
|
||
- 评审意见分为采纳和拒绝，延后=拒绝
|
||
|
||
> 来源：批次1卡片5（评审标准，freq=1）+ 批次1卡片12（不延后，freq=1）+ 批次2卡片5（闭环标准，freq=2）合并
|
||
|
||
---
|
||
|
||
### 6. 方案确认的完整闭环标准（severity: medium）
|
||
|
||
**场景**：多方案并行开发时，哪些方案算"完成"容易模糊。
|
||
|
||
**正确做法**：
|
||
**一键三连收尾标准**（用户明确定义）：
|
||
1. 改文档（设计/方案）
|
||
2. 改代码（实现）
|
||
3. **司马懿 review 并确认最终代码片段**
|
||
|
||
每步完成都要有明确证据（邮件编号、代码片段引用）。
|
||
|
||
**⚠️ 常见错误**：
|
||
- 只跟踪"方案评审"状态，没跟踪"最终代码确认"状态
|
||
- 未闭环的方案被标记为"已完成"
|
||
|
||
**关键细节**：
|
||
- 状态跟踪表要包含三列：方案评审 / 最终代码确认 / 状态
|
||
- 司马懿的确认必须是具体的（"代码片段与实际改动一致"），不是笼统的"通过"
|
||
|
||
> 来源：批次2卡片5（闭环标准，freq=2）
|
||
|
||
---
|
||
|
||
## 检查清单（快速参考）
|
||
|
||
- [ ] 评审请求是否包含最终代码（不是方案描述）？
|
||
- [ ] 状态/枚举值是否全部从集中定义取用？有无硬编码字符串？
|
||
- [ ] 状态转换是否只有一个入口函数？
|
||
- [ ] 设计文档是否逐条检查了实现覆盖度？有无遗漏的 § 节？
|
||
- [ ] 发现评审误判时，是否主动发出修正（而非静默修改）？
|
||
- [ ] 是否有不合理的"延后到下一版本"建议？用户偏好一步到位
|
||
- [ ] 一键三连是否闭环：文档→代码→最终代码确认？
|
||
- [ ] 评审结果处理完是否把修改后的代码再发评审者确认？
|