diff --git a/CODE_REVIEW_LESSONS.md b/CODE_REVIEW_LESSONS.md new file mode 100644 index 0000000..16b7ab7 --- /dev/null +++ b/CODE_REVIEW_LESSONS.md @@ -0,0 +1,126 @@ +# KWCode 代码审查经验总结 + +> 日期:2026-04-30 +> 触发:完整代码审查发现 8 个问题(3 个空架子、3 个功能矛盾、2 个数据错误) + +--- + +## 问题分类与根因 + +### 一、空架子问题(功能写了但跑不起来) + +| # | 问题 | 根因 | 修复 | +|---|------|------|------| +| 1 | DebugSubagent 永远不运行 | `main.py` 构建 orchestrator 时没传 `debug_subagent` 参数 | 实例化 DebugSubagent 并注入 | +| 2 | PromptOptimizer 永远不被调用 | 写了方法但没有任何调用点 | 接入 `check_graduation()` 投产流程 | +| 3 | Cross-Encoder 没人会装 | 可选依赖无提示 | 确认已有优雅降级,无需改 | + +**根因总结**:写新模块时只关注模块本身的实现,没有验证"谁来调用它"。模块写完了但没有接入调用链。 + +### 二、功能间矛盾(A 和 B 单独没问题,组合起来出错) + +| # | 问题 | 根因 | 修复 | +|---|------|------|------| +| 4 | Checkpoint + /multi 并行竞态 | 每个子任务独立 git stash,并行时互相覆盖 | 子任务级 `skip_checkpoint=True` | +| 5 | hard 任务没自动走 TaskPlanner | TaskPlanner 存在但只有 /multi 手动触发 | 待接入(下一步) | +| 6 | force_plan_mode 无法关闭 | 小模型策略强制 plan,用户无法覆盖 | 加 override 条件 | + +**根因总结**:功能是分批实现的(先做 Checkpoint,后做 /multi),没有回头检查新功能是否和旧功能冲突。缺少"组合测试"思维。 + +### 三、数据错误(代码能跑但数据是假的) + +| # | 问题 | 根因 | 修复 | +|---|------|------|------| +| 7 | conversation_history 存假数据 | `assistant` content 存的是 `user_input` 而非 LLM 输出 | 存真实 explanation | +| 8 | 多语言 AST 是空架子 | 代码已正确标注 Python-only,无虚假声明 | 无需修复 | + +**根因总结**:快速实现时用占位符(`user_input[:500]`)代替真实数据,后来忘了替换。 + +--- + +## 经验教训 + +### 1. 新模块必须验证调用链 + +**规则**:写完一个新模块后,必须在 `main.py` 或调用方里找到"谁实例化它、谁调用它"。 + +**检查清单**: +``` +□ 模块在 __init__ 里被实例化了吗? +□ 实例被传给了需要它的对象吗? +□ 有至少一个代码路径会触发它的核心方法吗? +□ 如果是可选功能,降级路径是否正确(不是 return None 导致后续 NPE)? +``` + +### 2. 并行功能必须检查共享状态 + +**规则**:任何涉及并行执行的功能,必须检查是否有共享可变状态(文件系统、git、数据库)。 + +**检查清单**: +``` +□ 并行任务是否操作同一个文件/目录? +□ 是否有 git 操作(stash/commit/checkout)在并行中执行? +□ 是否有全局变量被多线程同时写? +□ 如果有,是否需要锁、跳过、或改为串行? +``` + +### 3. 占位符必须标记 TODO + +**规则**:任何用占位符代替真实数据的地方,必须加 `# TODO: replace with real data` 注释。 + +**反例**: +```python +# 错误:看起来像完成了,实际是假数据 +conversation_history.append({"role": "assistant", "content": user_input[:500]}) + +# 正确:明确标记为占位 +conversation_history.append({"role": "assistant", "content": user_input[:500]}) # TODO: replace with real LLM output +``` + +### 4. 新功能实现后必须检查与现有功能的交互 + +**规则**:每次加新功能,列出它可能影响的现有功能,逐一检查。 + +**模板**: +``` +新功能:/multi 多任务并行 +可能影响: + □ Checkpoint(git stash)→ 并行时竞态? → 是,需要 skip + □ 重试机制 → 子任务各自重试? → 是,独立重试没问题 + □ 飞轮记录 → 每个子任务独立记录? → 是,没问题 + □ 状态栏显示 → 并行时显示哪个? → 需要处理 +``` + +### 5. 代码审查时的检查顺序 + +``` +第一遍:调用链完整性 + - 每个 __init__ 参数都有人传吗? + - 每个方法都有人调用吗? + - 可选参数的默认值(None)是否导致功能静默失效? + +第二遍:并行安全性 + - ThreadPoolExecutor 里的任务是否操作共享状态? + - 文件系统操作是否有竞态? + +第三遍:数据真实性 + - 日志/统计/历史记录里的数据是真实的还是占位符? + - 用户看到的数字(token数、耗时、成功率)是否准确? + +第四遍:功能组合 + - 新功能 × 旧功能的所有组合是否都测试过? + - 特别关注:并行×状态、重试×缓存、多任务×单任务 +``` + +--- + +## 后续防范措施 + +1. **每次新增模块后**:跑一遍"调用链检查"(从 main.py 出发,确认新模块被实例化和调用) +2. **每次加并行功能后**:列出所有共享状态,逐一确认安全 +3. **代码里禁止无标记占位符**:所有临时代码必须有 `# TODO` 或 `# PLACEHOLDER` +4. **功能组合矩阵**:维护一个简单的表格,新功能加入时检查与现有功能的交互 + +--- + +*本文档在每次代码审查后更新,积累经验防止重复犯错。*