diff --git a/docs/research/distill-skills-draft/code-review-quality.md b/docs/research/distill-skills-draft/code-review-quality.md new file mode 100644 index 0000000..7eb184e --- /dev/null +++ b/docs/research/distill-skills-draft/code-review-quality.md @@ -0,0 +1,172 @@ +--- +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) + +--- + +## 检查清单(快速参考) + +- [ ] 评审请求是否包含最终代码(不是方案描述)? +- [ ] 状态/枚举值是否全部从集中定义取用?有无硬编码字符串? +- [ ] 状态转换是否只有一个入口函数? +- [ ] 设计文档是否逐条检查了实现覆盖度?有无遗漏的 § 节? +- [ ] 发现评审误判时,是否主动发出修正(而非静默修改)? +- [ ] 是否有不合理的"延后到下一版本"建议?用户偏好一步到位 +- [ ] 一键三连是否闭环:文档→代码→最终代码确认? +- [ ] 评审结果处理完是否把修改后的代码再发评审者确认?