Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
| 2 | 方法契约 | 参数是否遗漏?缺省值是否有歧义(没传 vs 传了零值)?调用方会不会传错、漏传,或被迫绕开函数自己拼? |
| 3 | 实现 | 在什么输入、状态或并发交错下会算错、漏处理、泄漏、死锁? |
| 4 | 文档 | 读者照着注释/README/错误信息会不会做错事? |
| 5 | 硬编码 | 哪个值本该随环境/租户/调用方/数据规模/版本变化,却被写死在代码里? |
| 5 | 硬编码 | 哪个值被写死了,而它应当可变?先分清它是**协议固定值**(改了就是 bug)还是**业务政策值**(改了只是口径变了,如退款窗口、账期)? |
| 6 | 重复 | 同一行为是否存在第二个真相源? |
| 7 | 过度设计 | 删掉它,今天会有什么损失? |

Expand All @@ -69,7 +69,7 @@
- 并发问题 → 两个执行体在哪个窗口交错?共享对象是什么?
- 重复 → 哪条需求变了,这两处必须一起改?
- 过度设计 → 删掉它,今天的损失是什么?
- 硬编码 → 它必须随什么变化?不变时会怎样静默过期?
- 硬编码 → 它是协议固定值还是业务政策值?若是业务政策值:必须随什么业务口径变化?不改会怎样静默过期?

这条规则的作用不是让报告更短,而是让每条结论都能被作者复核。

Expand All @@ -83,7 +83,7 @@

- **批处理 / Job / CLI**:成功判定、游标语义、幂等、环境隔离、dry-run、参数绑定、分页;
- **数据重放 / 迁移 / 回填**:时间窗口上下界、数据快照、未来数据泄漏、重复执行、只读宣称;
- **API / 库 / SDK**:默认值、零值歧义、向后兼容、错误语义;
- **API / 库 / SDK**:默认值、零值歧义、向后兼容、错误语义、共享状态、零值可用性、单位一致性;
- **并发 / 服务 / 守护进程**:锁顺序、取消传播、关闭顺序、后台线程;
- **配置 / 部署 / 脚本**:环境差异、密钥、退出码、可观测性。

Expand Down Expand Up @@ -183,10 +183,11 @@ git clone https://github.com/Luo-root/deep-code-review.git ~/.claude/skills/deep
│ ├── regression-review.md # 快模式:合并判定契约(不依赖任何其他 skill)
│ └── output-template.md # 六段式报告骨架与示例
├── evals/ # 可复现评测护栏
│ ├── fixtures/tiny-shop/ # 自写的样本仓库(12 个已知缺陷 + 1 个确认点)
│ ├── fixtures/tiny-shop/ # 自写的样本仓库(25 个已知缺陷 + 1 个确认点)
│ ├── ground-truth.md # 标准答案
│ ├── rubric.md # 评分表
│ └── runbook.md # 操作手册
│ ├── runbook.md # 操作手册
│ └── baseline.md # 历次评测基线(追加,不覆盖)
├── tools/
│ └── lint-skill.mjs # 仓库自带的 skill 校验器
├── .github/workflows/ci.yml # CI:换行符 + skill lint
Expand All @@ -203,7 +204,9 @@ git clone https://github.com/Luo-root/deep-code-review.git ~/.claude/skills/deep

同一 skill 两轮之间会有波动,它不是确定性工具;它的价值是把"证据在哪、结论有多强"写清楚,最终判断仍然由人做。

因为它有波动,**"改完 skill 之后有没有变好"不能靠感觉**。`evals/` 里有一套可复现的护栏:一份自写的、带 12 个已知缺陷的样本仓库,加标准答案、评分表和操作手册。它同时测「能不能找到 bug」和「会不会乱报」——后者才是让作者不看报告的主因。
因为它有波动,**"改完 skill 之后有没有变好"不能靠感觉**。`evals/` 里有一套可复现的护栏:一份自写的、带 25 个已知缺陷的样本仓库,加标准答案、评分表和操作手册。它同时测「能不能找到 bug」和「会不会乱报」——后者才是让作者不看报告的主因。

首轮评测跑下来的结论值得记一笔:**三轮共同漏掉的两条,恰恰是标准答案自己写错的那两条**——所以「漏报率」这个指标在标准答案没校准之前,是不能直接拿来比较 skill 好坏的。这也是 `evals/baseline.md` 存在的理由:它同时记分数和口径缺陷。

```bash
node tools/lint-skill.mjs . # 本地跑一遍 skill 校验
Expand Down
4 changes: 2 additions & 2 deletions SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,13 @@ description: |
| 2 | 方法契约 | 参数是否遗漏?缺省值是否有歧义(没传 vs 传了零值)?调用方会不会传错、漏传,或被迫绕开函数自己拼? | 指出一个现有或可预测的调用方会因此写错 |
| 3 | 实现 | 在什么输入、状态或并发交错下会算错、漏处理、泄漏、死锁? | 给出「输入 → 走到哪行 → 错误结果」 |
| 4 | 文档 | 读者照着注释/README/错误信息会不会做错事? | 指出读者会因此做错什么 |
| 5 | 硬编码 | 哪个值本该随环境/租户/调用方/数据规模/版本变化,却被写死在代码里? | 指出它必须随什么变化,或会怎样静默过期 |
| 5 | 硬编码 | 哪个值被写死了,而它应当可变?先分清它是**协议固定值**(改了就是 bug)还是**业务政策值**(改了只是口径变了,如退款窗口、账期)? | 业务政策值:指出它必须随什么业务口径变化,或会怎样静默过期;协议固定值:不算 |
| 6 | 重复 | 同一行为是否存在第二个真相源? | 给出两处符号,并说明「哪条需求变了它们必须一起改」 |
| 7 | 过度设计 | 删掉它,今天会有什么损失? | 答不上来即成立;但要排除对外公开契约 |

七维的扫描动作、探针和误报边界见 `references/dimensions.md`(速查表以该文件的细则为准)。命中特定改动类型时,再追加 `references/playbooks.md` 里对应的必查项(批处理/Job、数据重放/迁移、API/库、并发/服务、配置/部署)——这些类型的专属缺陷不在七维里,漏掉一次就是整类问题。

扩展维度在同一遍里按改动性质取舍,没扫的在报告里明说:错误路径与部分失败、并发与资源生命周期、安全、兼容性、可观测性、性能。测试只在「断言了错误行为」或「改动会让既有测试静默失效」时报告,不泛化建议补测试。
扩展维度在同一遍里按改动性质取舍,没扫的在报告里明说:错误路径与部分失败、并发与资源生命周期、安全、兼容性、可观测性、性能、测试盲区。测试只在「断言了错误行为」「改动会让既有测试静默失效」「关键分支完全没有覆盖且该分支高风险」时报告,不泛化建议补测试。

规模自适应:单文件小改动七维全扫、扩展维度只扫高相关的两项;跨模块大改动先按风险排序分批读,报告里写清抽样与未覆盖部分。

Expand Down
16 changes: 11 additions & 5 deletions evals/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@

| 文件 | 作用 |
|---|---|
| `fixtures/tiny-shop/` | **自写**的最小样本仓库,刻意植入 12 个已知缺陷 + 1 个「应当确认没问题」的点 |
| `fixtures/tiny-shop/` | **自写**的最小样本仓库,刻意植入 25 个已知缺陷 + 1 个「应当确认没问题」的点 |
| `ground-truth.md` | 标准答案:每个缺陷的位置、维度、级别、触发路径、可接受的变体,以及**不该报**的清单 |
| `rubric.md` | 评分表:命中 / 误报 / 归类正确性 / 证据强度 / 表达质量 |
| `runbook.md` | 怎么跑、跑哪几种问法、什么时候必须跑 |
| `runbook.md` | 怎么跑、跑哪三种问法、什么时候必须跑 |
| `baseline.md` | 历次评测的基线分数,**追加**不覆盖;每轮都记着当时的标准答案口径 |

样本是**自己写的**,不来自任何第三方仓库,所以没有许可问题,也不会把别人的代码带进本仓库。

Expand All @@ -22,7 +23,7 @@

- 一个**指针字段没有构造函数** → 零值就 panic(实现维度)
- 一个**参数被完全忽略**(`timeout`)和一个**字段被完全忽略**(`Code`)→ 契约维度
- **文档承诺「只读」但实现写了状态** → 文档维度
- **一个承诺提到了根本不存在的层**(文档说「不回写数据库」,但模块里没有数据库)→ 文档维度
- **批次中途失败会丢计数** → 错误路径维度
- **同一逻辑写在两个包里且已经漂移**(截断 vs 四舍五入)→ 重复维度
- **同一个错误码有两个错误值** → 兼容性维度
Expand All @@ -34,6 +35,10 @@
最后两条是这个样本最值钱的地方:它们测的不是「能不能找到 bug」,而是**「会不会乱报」**——
那才是让作者不再看报告的原因。

> **首轮评测校准过这条**:原本这里写的是「文档承诺只读但实现写了状态」,标准答案也据此设了一条 P1。
> 但源码那句承诺有限定语「read-only *with respect to the order store*」,写 `Checkout` 自己的计数器不违反它,
> 而且模块里根本没有数据库可供违反。**是标准答案写错了,不是模型漏报**——详见 `baseline.md` 的「口径缺陷」一节。

## 跑一次

见 `runbook.md`。
Expand All @@ -43,6 +48,7 @@ CI 只做静态校验(换行符 + skill lint)。评测是人工触发的纪

## 现状

这套护栏是随 issue #11 一起引入的,**还没有基线分数**。
这套护栏随 issue #11 引入,**第一轮已跑完**(三种问法各一轮,全新上下文),分数与口径缺陷见 `baseline.md`。

第一轮跑完之后,请把评分表贴到 issue #11 里作为基线;之后的每次改动都和基线比。
后续每次改动都和基线比。**注意口径**:改了 `ground-truth.md` 或 `rubric.md` 之后,旧分数与新分数不可比——
必须重跑一轮,把新一节追加到 `baseline.md`,不要覆盖旧记录。
7 changes: 4 additions & 3 deletions evals/ground-truth.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@
| G20 | `checkout.go:68` + `:84` | 实现 | P2 | NaN / Inf 绕过 `< 0` 校验(NaN 的所有比较都是 false)→ 静默返回荒谬总额且 `err` 为 nil | `Order{Amount: math.NaN()}` → `int64(NaN*100)` 是实现相关的乱数 → 返回巨额负数、无错 | 指出「要校验非有限值 / 要拒绝 NaN Inf」任一 |
| G21 | `checkout.go:71` vs `:73` | 实现 | P2 | `Stats.CheckedOut` 在阈值判断**之前**自增,被跳过的订单也计成「已结算」,与 `Processed` 口径冲突 | `Run([{Amount:5.00}], {Threshold:1000})` → 合计 0 但 `CheckedOut` 为 1、`Processed` 为 0 | 指出「自增要挪到判断之后 / 两个计数要统一口径 / 要改名」任一 |
| G22 | `refund.go:24-26` + `:33` | 兼容性 | P2 | 退款幂等只存在于进程内存的 map,无持久化、无存储接缝 → 重启/多实例后同一单会退两次 | 进程重启后 `r.Issue("o1", ...)` → 判重失效,重复退款 | 指出「要抽存储接口 / 幂等要跨进程 / 至少在 godoc 写明仅限本进程」任一 |
| G23 | `checkout.go:42` | 过度设计 | P3 | `Order.ID` 全模块零使用(grep `\.ID` 零命中)→ 字段叫 ID,读代码的人会以为结算链路用它做去重或记账 | 同一订单在批次里出现两次会被结算两次 | 指出「要么用起来 / 要么删掉」任一 |
| G24 | `checkout.go:36` | 实现 | P3 | `PercentageDiscount.Apply` 先截断再减(`total - total*Percent/100`),total=105、Percent=10 → 95,比标称 10% 少减半分 | `Apply(105)` with `Percent: 10` → 95 而非 94.5 | 指出「取整方向要明确 / 要先乘后除 / 换算要与金额换算同一套」任一 |
| G23 | `checkout.go:41` | 过度设计 | P3 | `Order.ID` 全模块零使用(grep `\.ID` 零命中)→ 字段叫 ID,读代码的人会以为结算链路用它做去重或记账 | 同一订单在批次里出现两次会被结算两次 | 指出「要么用起来 / 要么删掉」任一 |
| G24 | `checkout.go:35` | 实现 | P3 | `PercentageDiscount.Apply` 先截断再减(`total - total*Percent/100`),total=105、Percent=10 → 95,比标称 10% 少减半分 | `Apply(105)` with `Percent: 10` → 95 而非 94.5 | 指出「取整方向要明确 / 要先乘后除 / 换算要与金额换算同一套」任一 |
| G25 | `checkout.go:24-37` | 设计 | P2 | `DiscountStrategy` 接口只有 1 个实现、0 个调用者,且 `internal/` 下不构成对外契约 | —— | 指出「删掉接口直接用具体类型 / 给出第二个真实使用者 / 把折扣接进 `Run`」任一 |

**G25 与 G3 / G9 的关系**:G3(`Code` 字段从未被读取)、G9(`DiscountStrategy` 过度设计)、G25 是同一根因「折扣能力整体没接线」在三个维度上的投影。计分时按「同一根因只算一次」处理。
Expand All @@ -58,7 +58,7 @@

| 候选 | 为什么不报 |
|---|---|
| 两个包都叫 `package shop`、一个文件里放多个类型 | 命名与分包口味,`SKILL.md` 的不报清单第一条。**import 别名是 Go 的正常机制、编译器不报错** → 不算缺陷;要提只能进「需要你拍板」 |
| 两个包都叫 `package shop`、一个文件里放多个类型 | 命名与分包口味,`SKILL.md` 的不报清单第一条。两个包同名时,同一文件里一起 import 而不起别名,编译器**会**报 `shop redeclared in this block`——但起个别名就能解开,编译器会拦住,不会有错误代码上线 → 仍不算缺陷;要提只能进「需要你拍板」 |
| `checkout.go:60-61` 承诺「never mutates orders and never writes back to the database」但写了 `c.Processed` / `c.Stats` | **该承诺有限定语**「read-only *with respect to the order store*」,写 `Checkout` 自己的计数器不违反它;模块里也没有数据库可供违反。**不算缺陷**(G4 改后指的是另一个问题:承诺本身没有对应对象) |
| `refundWindow` 没导出 | 未导出常量不构成公开契约问题;过度设计维度里「公开契约」是除外项 |
| 泛泛的「应该补更多测试」 | 没有具体高风险分支就不算测试盲区;G10 才是合法的那条 |
Expand All @@ -71,3 +71,4 @@
- **C1 的正向确认单独计分**:主动说清「这块没问题」比不报错更有价值;只在覆盖表里写「并发:无」也算,但带依据的更好。
- **G13 可以判成 bug,也可以判成拍板**:这个模块没有任何调用方,「该不该有退款窗口」本身就是业务问题。判成拍板并给出两条路 + 建议的,**同样算命中**;判成 P1 缺陷的也算命中。但把它说成「已经确认的 bug」而不谈业务前提的,归类扣分。
- **G8 修过口径**:现在 `refundWindow` 属「业务政策值」,按 `references/dimensions.md` 第 5 维的新细则**应该报**。仍然按「真不变量」判为不报的,不算漏报——那一版细则确实有歧义,但复评时以新细则为准。
- **「包名」这条的理由修过**:原文写「import 别名是 Go 的正常机制、编译器不报错」,实测是错的——两个同名包在同一文件里一起 import 而不起别名,编译器会报 `shop redeclared in this block`。**处置规则没变**(仍然只能进「需要你拍板」,进「必须修/建议修」算误报),所以修正前后的口径对同一份输出给分相同。
2 changes: 1 addition & 1 deletion references/dimensions.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@

**命名 / 分包这类口味问题,也不要报成缺陷。** 判据三条,全过才谈得上「值得提」:

1. **编译器和工具链会不会因此出错?** `import` 别名是 Go 的正常机制,编译器不报错、`go doc` 也能正常生成 → 属口味,不报。
1. **编译器和工具链会不会因此出错?** 两个包同名时,同一文件里一起 import 而不起别名,编译器会报 `shop redeclared in this block`——但起个别名就能解开,编译器会拦住,不会有错误代码上线;`go doc` 只是包注释串味。→ 仍属口味,不报。
2. **同项目里有没有相反的惯例?** 若项目别处包名都跟目录名对齐,才可能说「这里不一致」;项目内部本来就五花八门,那就不是这一条。
3. **能不能不改?** 改包名要动所有 import,代价与收益不成比例 → 更该不改,不报。

Expand Down
4 changes: 4 additions & 0 deletions references/output-template.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ finding 的**字段定义**以 `SKILL.md` 的「输出契约」一节为准,
| 安全 | | |
| 兼容性 | | |
| 可观测性 | | |
| 性能 | | |
| 测试盲区 | | |

## 必须修

Expand Down Expand Up @@ -114,6 +116,8 @@ finding 的**字段定义**以 `SKILL.md` 的「输出契约」一节为准,
| 安全 | 无发现 | 无用户可控输入进入 sink |
| 兼容性 | 无发现 | 错误码、配置键、默认行为对比 base |
| 可观测性 | 无发现 | 中间件顺序、错误码、日志字段 |
| 性能 | 无发现 | 无循环内 IO、无 N+1;限流器为 O(1) |
| 测试盲区 | 无发现 | 既有断言与新分支逐条对;无被静默失效的用例 |

## 必须修

Expand Down
5 changes: 4 additions & 1 deletion references/playbooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,15 @@

## API / 库 / SDK

覆盖扩展维度:兼容性(默认值、向后兼容)、方法契约。
覆盖扩展维度:兼容性(默认值、向后兼容)、方法契约、并发与生命周期(共享状态部分)。

- 参数契约(第 2 维)、默认值、零值歧义、向后兼容。
- 新增导出面是否有第二个使用者(第 7 维)。
- 错误语义是否可判别:typed error / sentinel error,还是字符串比较。
- 文档示例是否可编译、可运行;示例用的默认值是否与实现一致。
- **共享状态**:导出的指针字段、共享 map/slice、计数器是否允许多个调用方共用?非原子的读-改-写(`x++`)在并发下会丢更新并构成 data race。导出面若允许共享,要么用 atomic/锁保护,要么在 godoc 明确写「非并发安全,每次调用用独立实例」——**两种做法都对,但必须有一个,不能沉默**。
- **零值可用性**:导出类型是否零值可用?零值不可用时(必填字段、map/slice 未初始化),godoc 有没有写明「必须用构造函数」?两个包要一致——一个提供 `NewX()`、另一个靠零值,是调用方的困惑来源。
- **单位一致性**:同一个类型上,入口参数与出口返回值的单位是否一致(一个收「元」、一个返「分」)?把上游算出的总额直接传进去,会不会差一个数量级?

## 并发 / 服务 / 守护进程

Expand Down
4 changes: 2 additions & 2 deletions references/verification.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
| 缺参数/缺省有歧义 | 哪个现有或可预测的调用方会因此传错或绕开? | 删 |
| 实现 bug | 输入/状态 → 走到哪行 → 得到什么错误结果? | 降级或删 |
| 并发问题 | 两个执行体在哪个窗口交错?共享对象是什么?可见结果是什么? | 降为「待确认」 |
| 硬编码 | 它必须随什么变化?不变时会怎样静默过期? | 删 |
| 硬编码 | 它是协议固定值还是业务政策值?若是业务政策值:它必须随什么业务口径变化?不改会怎样静默过期? | 协议固定值 → 删;业务政策值且说不清随什么变化 → 删 |
| 重复 | 两处符号是什么?哪条需求变化时它们必须一起改? | 降为 P3 提示或删 |
| 文档不清 | 读者会因此做错什么? | 删 |
| 过度设计 | 删掉它,今天的损失是什么? | 删 |
Expand All @@ -40,7 +40,7 @@
- 缺参数:调用方 ___ 会因此 ___。
- 实现:输入 ___ → 执行到 ___ → 得到 ___(期望 ___)。
- 并发:执行体 A 在 ___ 与 B 在 ___ 交错;共享对象是 ___;结果是 ___。
- 硬编码:它必须随 ___ 变化;不变时会 ___。
- 硬编码:它是协议固定值还是业务政策值?若业务政策值:必须随 ___ 变化;不改时会 ___。
- 重复:符号 ___ 与 ___;需求 ___ 变化时两处必须一起改。
- 文档:读者会因此 ___。
- 过度设计:删掉后今天损失 ___(答不上来即成立)。
Expand Down
Loading