From 928b8e127c71a4b6a687a52cd2550814dede98b3 Mon Sep 17 00:00:00 2001 From: cfdaily Date: Wed, 27 May 2026 00:10:01 +0800 Subject: [PATCH] auto-sync: 2026-05-27 00:10:01 --- skills/review-quality.md | 58 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 skills/review-quality.md diff --git a/skills/review-quality.md b/skills/review-quality.md new file mode 100644 index 0000000..86454d7 --- /dev/null +++ b/skills/review-quality.md @@ -0,0 +1,58 @@ +--- +name: review-quality +description: >- + 代码评审的最佳实践:闭环标准、枚举一致性、三层对照、自我纠正。 + 在执行代码评审、或准备提交评审时触发。 +--- +# Review Quality(评审质量) + +> 来源:从司马懿 122 次纠正和庞统评审经验中蒸馏(卡片 1、2、3、4、5) + +## 适用场景 + +- 执行代码评审时(作为评审者) +- 准备提交代码给评审者时(作为被评审者) +- 设计文档与代码实现的对齐验证 +- 多方案并行开发时的闭环跟踪 + +## 模式清单 + +### 模式 1:一键三连闭环 — 文档→代码→最终代码确认 + +评审不是止于方案确认。完整闭环是三步:改文档(设计/方案)→ 改代码(实现)→ 评审者确认最终代码片段。"已采纳建议"不等于"已验证"——采纳后的代码可能引入新问题。评审者的确认必须是具体的(引用代码行号,确认与实际改动一致),不是笼统的"通过"。 + +> **Evidence**: 卡片 1 — 庞统让司马懿 review 了方案,但实现后没发最终代码确认。用户指出"一键三连最终肯定是以司马懿 review 完最终成果物收尾的"。卡片 5 — 状态跟踪表应包含三列:方案评审 / 最终代码确认 / 状态。 + +### 模式 2:枚举值一致性审查 — 零容忍硬编码字符串 + +状态机/枚举值不一致是高频 Bug 根源。审查时重点检查:新代码中的状态值是否从枚举定义取值(硬编码字符串零容忍)、状态转换是否只有一个入口函数(如 `_transition_status`)、所有 spawn/查询/恢复路径是否使用相同的枚举引用。发现不一致时标记为 high severity。 + +> **Evidence**: 卡片 2 — `get_all_executing_tasks` SQL 用 `'executing'` 但实际枚举是 `'working'`,导致重启恢复遗漏 working 节点(P0-2)。`max_per_agent=2` vs 设计文档的 `1` 也是同类问题。 + +### 模式 3:三层对照审查 — PRD→设计→代码逐条覆盖 + +每次代码评审前,先对设计文档逐条检查实现覆盖度。设计文档用明确编号(§B1/B2/B3)便于逐条追踪。评审者的核心职责就是做这种三层对照:PRD 要求了什么 → 设计文档怎么规划的 → 代码实现了多少。遗漏一个章节就等于功能缺失。 + +> **Evidence**: 卡片 4 — spawner-monitor 设计文档 §B2(compact 进行中继续等)在代码中完全遗漏,直到庞统重新读设计文档才暴露。庞统专门发 #311 邮件请求"PRD→设计→代码三层一致性审查"。 + +### 模式 4:自我纠正机制 — 承认误判比坚持错误更有价值 + +评审中发现自己的判断有误时,主动发出修正并明确标注。交叉验证是发现误判的有效手段:翻回设计文档对照,看代码实际行为 vs 自己的判断。庞统和司马懿在同一次评审中都做了自我纠正——这说明交叉验证机制在工作。修正保留审计轨迹,不能静默修改。 + +> **Evidence**: 卡片 3 — 庞统修正"建议删除 recent_compact"为"设计文档明确要求";司马懿回复"你说得对,我判断错了"。 + +### 模式 5:评审前验证实测数据,不接受"应该是" + +涉及外部命令输出解析、API 返回结构、配置文件内容时,评审者应要求看到实测输出样例,而非接受代码中的假设路径。"从第一天就存在的根因 bug"往往源于解析路径是猜的而非验证的。评审时把"需实测验证"作为一条显式检查项。 + +> **Evidence**: 卡片 1 — 司马懿多次在代码评审中发现方案评审没暴露的 Bug(如 `_do_retry` 续杯失效),靠的是看最终代码而非方案描述。卡片 2 — auto-sync 可能静默修改配置文件,需要与设计文档对照验证。 + +## 检查清单 + +- [ ] 方案评审通过后,是否安排了最终代码确认环节? +- [ ] 状态/枚举值是否全部从集中定义取值,无硬编码字符串? +- [ ] 设计文档是否逐条(按编号)检查了实现覆盖度? +- [ ] 发现自己判断有误时,是否主动发出了修正? +- [ ] 外部输出解析路径是否提供了实测样例验证? +- [ ] 评审结论是否具体到代码行号,而非笼统"通过"? +- [ ] 所有 spawn/查询/恢复路径是否覆盖了新功能的参数传递?