Files
kwcode/CODE_REVIEW_LESSONS.md
Val-sss 8dfb962515 docs: add CODE_REVIEW_LESSONS.md — post-mortem and prevention rules
Summarizes 8 issues found in code review:
- 3 dead code (module exists but never called)
- 3 feature conflicts (parallel race, override missing)
- 2 data errors (placeholder never replaced)

Establishes 5 prevention rules:
1. New module → verify call chain (who instantiates, who calls)
2. Parallel feature → check shared mutable state
3. Placeholder → must have TODO comment
4. New feature → check interaction with all existing features
5. Review order: call chain → parallel safety → data truth → combinations

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-30 03:00:51 +08:00

5.0 KiB
Raw Permalink Blame History

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 注释。

反例

# 错误:看起来像完成了,实际是假数据
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 多任务并行
可能影响:
  □ Checkpointgit stash→ 并行时竞态? → 是,需要 skip
  □ 重试机制 → 子任务各自重试? → 是,独立重试没问题
  □ 飞轮记录 → 每个子任务独立记录? → 是,没问题
  □ 状态栏显示 → 并行时显示哪个? → 需要处理

5. 代码审查时的检查顺序

第一遍:调用链完整性
  - 每个 __init__ 参数都有人传吗?
  - 每个方法都有人调用吗?
  - 可选参数的默认值None是否导致功能静默失效

第二遍:并行安全性
  - ThreadPoolExecutor 里的任务是否操作共享状态?
  - 文件系统操作是否有竞态?

第三遍:数据真实性
  - 日志/统计/历史记录里的数据是真实的还是占位符?
  - 用户看到的数字token数、耗时、成功率是否准确

第四遍:功能组合
  - 新功能 × 旧功能的所有组合是否都测试过?
  - 特别关注:并行×状态、重试×缓存、多任务×单任务

后续防范措施

  1. 每次新增模块后:跑一遍"调用链检查"(从 main.py 出发,确认新模块被实例化和调用)
  2. 每次加并行功能后:列出所有共享状态,逐一确认安全
  3. 代码里禁止无标记占位符:所有临时代码必须有 # TODO# PLACEHOLDER
  4. 功能组合矩阵:维护一个简单的表格,新功能加入时检查与现有功能的交互

本文档在每次代码审查后更新,积累经验防止重复犯错。