diff --git a/README.md b/README.md index ca26a62..eeb7936 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,7 @@ | 2 | 方法契约 | 参数是否遗漏?缺省值是否有歧义(没传 vs 传了零值)?调用方会不会传错、漏传,或被迫绕开函数自己拼? | | 3 | 实现 | 在什么输入、状态或并发交错下会算错、漏处理、泄漏、死锁? | | 4 | 文档 | 读者照着注释/README/错误信息会不会做错事? | -| 5 | 硬编码 | 哪个值本该随环境/租户/调用方/数据规模/版本变化,却被写死在代码里? | +| 5 | 硬编码 | 哪个值被写死了,而它应当可变?先分清它是**协议固定值**(改了就是 bug)还是**业务政策值**(改了只是口径变了,如退款窗口、账期)? | | 6 | 重复 | 同一行为是否存在第二个真相源? | | 7 | 过度设计 | 删掉它,今天会有什么损失? | @@ -69,7 +69,7 @@ - 并发问题 → 两个执行体在哪个窗口交错?共享对象是什么? - 重复 → 哪条需求变了,这两处必须一起改? - 过度设计 → 删掉它,今天的损失是什么? -- 硬编码 → 它必须随什么变化?不变时会怎样静默过期? +- 硬编码 → 它是协议固定值还是业务政策值?若是业务政策值:必须随什么业务口径变化?不改会怎样静默过期? 这条规则的作用不是让报告更短,而是让每条结论都能被作者复核。 @@ -83,7 +83,7 @@ - **批处理 / Job / CLI**:成功判定、游标语义、幂等、环境隔离、dry-run、参数绑定、分页; - **数据重放 / 迁移 / 回填**:时间窗口上下界、数据快照、未来数据泄漏、重复执行、只读宣称; -- **API / 库 / SDK**:默认值、零值歧义、向后兼容、错误语义; +- **API / 库 / SDK**:默认值、零值歧义、向后兼容、错误语义、共享状态、零值可用性、单位一致性; - **并发 / 服务 / 守护进程**:锁顺序、取消传播、关闭顺序、后台线程; - **配置 / 部署 / 脚本**:环境差异、密钥、退出码、可观测性。 @@ -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 @@ -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 校验 diff --git a/SKILL.md b/SKILL.md index 92d74ea..24113d4 100644 --- a/SKILL.md +++ b/SKILL.md @@ -91,13 +91,13 @@ description: | | 2 | 方法契约 | 参数是否遗漏?缺省值是否有歧义(没传 vs 传了零值)?调用方会不会传错、漏传,或被迫绕开函数自己拼? | 指出一个现有或可预测的调用方会因此写错 | | 3 | 实现 | 在什么输入、状态或并发交错下会算错、漏处理、泄漏、死锁? | 给出「输入 → 走到哪行 → 错误结果」 | | 4 | 文档 | 读者照着注释/README/错误信息会不会做错事? | 指出读者会因此做错什么 | -| 5 | 硬编码 | 哪个值本该随环境/租户/调用方/数据规模/版本变化,却被写死在代码里? | 指出它必须随什么变化,或会怎样静默过期 | +| 5 | 硬编码 | 哪个值被写死了,而它应当可变?先分清它是**协议固定值**(改了就是 bug)还是**业务政策值**(改了只是口径变了,如退款窗口、账期)? | 业务政策值:指出它必须随什么业务口径变化,或会怎样静默过期;协议固定值:不算 | | 6 | 重复 | 同一行为是否存在第二个真相源? | 给出两处符号,并说明「哪条需求变了它们必须一起改」 | | 7 | 过度设计 | 删掉它,今天会有什么损失? | 答不上来即成立;但要排除对外公开契约 | 七维的扫描动作、探针和误报边界见 `references/dimensions.md`(速查表以该文件的细则为准)。命中特定改动类型时,再追加 `references/playbooks.md` 里对应的必查项(批处理/Job、数据重放/迁移、API/库、并发/服务、配置/部署)——这些类型的专属缺陷不在七维里,漏掉一次就是整类问题。 -扩展维度在同一遍里按改动性质取舍,没扫的在报告里明说:错误路径与部分失败、并发与资源生命周期、安全、兼容性、可观测性、性能。测试只在「断言了错误行为」或「改动会让既有测试静默失效」时报告,不泛化建议补测试。 +扩展维度在同一遍里按改动性质取舍,没扫的在报告里明说:错误路径与部分失败、并发与资源生命周期、安全、兼容性、可观测性、性能、测试盲区。测试只在「断言了错误行为」「改动会让既有测试静默失效」「关键分支完全没有覆盖且该分支高风险」时报告,不泛化建议补测试。 规模自适应:单文件小改动七维全扫、扩展维度只扫高相关的两项;跨模块大改动先按风险排序分批读,报告里写清抽样与未覆盖部分。 diff --git a/evals/README.md b/evals/README.md index 49d75d0..082897f 100644 --- a/evals/README.md +++ b/evals/README.md @@ -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` | 历次评测的基线分数,**追加**不覆盖;每轮都记着当时的标准答案口径 | 样本是**自己写的**,不来自任何第三方仓库,所以没有许可问题,也不会把别人的代码带进本仓库。 @@ -22,7 +23,7 @@ - 一个**指针字段没有构造函数** → 零值就 panic(实现维度) - 一个**参数被完全忽略**(`timeout`)和一个**字段被完全忽略**(`Code`)→ 契约维度 -- **文档承诺「只读」但实现写了状态** → 文档维度 +- **一个承诺提到了根本不存在的层**(文档说「不回写数据库」,但模块里没有数据库)→ 文档维度 - **批次中途失败会丢计数** → 错误路径维度 - **同一逻辑写在两个包里且已经漂移**(截断 vs 四舍五入)→ 重复维度 - **同一个错误码有两个错误值** → 兼容性维度 @@ -34,6 +35,10 @@ 最后两条是这个样本最值钱的地方:它们测的不是「能不能找到 bug」,而是**「会不会乱报」**—— 那才是让作者不再看报告的原因。 +> **首轮评测校准过这条**:原本这里写的是「文档承诺只读但实现写了状态」,标准答案也据此设了一条 P1。 +> 但源码那句承诺有限定语「read-only *with respect to the order store*」,写 `Checkout` 自己的计数器不违反它, +> 而且模块里根本没有数据库可供违反。**是标准答案写错了,不是模型漏报**——详见 `baseline.md` 的「口径缺陷」一节。 + ## 跑一次 见 `runbook.md`。 @@ -43,6 +48,7 @@ CI 只做静态校验(换行符 + skill lint)。评测是人工触发的纪 ## 现状 -这套护栏是随 issue #11 一起引入的,**还没有基线分数**。 +这套护栏随 issue #11 引入,**第一轮已跑完**(三种问法各一轮,全新上下文),分数与口径缺陷见 `baseline.md`。 -第一轮跑完之后,请把评分表贴到 issue #11 里作为基线;之后的每次改动都和基线比。 +后续每次改动都和基线比。**注意口径**:改了 `ground-truth.md` 或 `rubric.md` 之后,旧分数与新分数不可比—— +必须重跑一轮,把新一节追加到 `baseline.md`,不要覆盖旧记录。 diff --git a/evals/ground-truth.md b/evals/ground-truth.md index df9a06d..b898026 100644 --- a/evals/ground-truth.md +++ b/evals/ground-truth.md @@ -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 是同一根因「折扣能力整体没接线」在三个维度上的投影。计分时按「同一根因只算一次」处理。 @@ -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 才是合法的那条 | @@ -71,3 +71,4 @@ - **C1 的正向确认单独计分**:主动说清「这块没问题」比不报错更有价值;只在覆盖表里写「并发:无」也算,但带依据的更好。 - **G13 可以判成 bug,也可以判成拍板**:这个模块没有任何调用方,「该不该有退款窗口」本身就是业务问题。判成拍板并给出两条路 + 建议的,**同样算命中**;判成 P1 缺陷的也算命中。但把它说成「已经确认的 bug」而不谈业务前提的,归类扣分。 - **G8 修过口径**:现在 `refundWindow` 属「业务政策值」,按 `references/dimensions.md` 第 5 维的新细则**应该报**。仍然按「真不变量」判为不报的,不算漏报——那一版细则确实有歧义,但复评时以新细则为准。 +- **「包名」这条的理由修过**:原文写「import 别名是 Go 的正常机制、编译器不报错」,实测是错的——两个同名包在同一文件里一起 import 而不起别名,编译器会报 `shop redeclared in this block`。**处置规则没变**(仍然只能进「需要你拍板」,进「必须修/建议修」算误报),所以修正前后的口径对同一份输出给分相同。 diff --git a/references/dimensions.md b/references/dimensions.md index 2bc213e..aa27f92 100644 --- a/references/dimensions.md +++ b/references/dimensions.md @@ -94,7 +94,7 @@ **命名 / 分包这类口味问题,也不要报成缺陷。** 判据三条,全过才谈得上「值得提」: -1. **编译器和工具链会不会因此出错?** `import` 别名是 Go 的正常机制,编译器不报错、`go doc` 也能正常生成 → 属口味,不报。 +1. **编译器和工具链会不会因此出错?** 两个包同名时,同一文件里一起 import 而不起别名,编译器会报 `shop redeclared in this block`——但起个别名就能解开,编译器会拦住,不会有错误代码上线;`go doc` 只是包注释串味。→ 仍属口味,不报。 2. **同项目里有没有相反的惯例?** 若项目别处包名都跟目录名对齐,才可能说「这里不一致」;项目内部本来就五花八门,那就不是这一条。 3. **能不能不改?** 改包名要动所有 import,代价与收益不成比例 → 更该不改,不报。 diff --git a/references/output-template.md b/references/output-template.md index e38f97b..bf09e86 100644 --- a/references/output-template.md +++ b/references/output-template.md @@ -27,6 +27,8 @@ finding 的**字段定义**以 `SKILL.md` 的「输出契约」一节为准, | 安全 | | | | 兼容性 | | | | 可观测性 | | | +| 性能 | | | +| 测试盲区 | | | ## 必须修 @@ -114,6 +116,8 @@ finding 的**字段定义**以 `SKILL.md` 的「输出契约」一节为准, | 安全 | 无发现 | 无用户可控输入进入 sink | | 兼容性 | 无发现 | 错误码、配置键、默认行为对比 base | | 可观测性 | 无发现 | 中间件顺序、错误码、日志字段 | +| 性能 | 无发现 | 无循环内 IO、无 N+1;限流器为 O(1) | +| 测试盲区 | 无发现 | 既有断言与新分支逐条对;无被静默失效的用例 | ## 必须修 diff --git a/references/playbooks.md b/references/playbooks.md index 2d104be..60a041f 100644 --- a/references/playbooks.md +++ b/references/playbooks.md @@ -34,12 +34,15 @@ ## API / 库 / SDK -覆盖扩展维度:兼容性(默认值、向后兼容)、方法契约。 +覆盖扩展维度:兼容性(默认值、向后兼容)、方法契约、并发与生命周期(共享状态部分)。 - 参数契约(第 2 维)、默认值、零值歧义、向后兼容。 - 新增导出面是否有第二个使用者(第 7 维)。 - 错误语义是否可判别:typed error / sentinel error,还是字符串比较。 - 文档示例是否可编译、可运行;示例用的默认值是否与实现一致。 +- **共享状态**:导出的指针字段、共享 map/slice、计数器是否允许多个调用方共用?非原子的读-改-写(`x++`)在并发下会丢更新并构成 data race。导出面若允许共享,要么用 atomic/锁保护,要么在 godoc 明确写「非并发安全,每次调用用独立实例」——**两种做法都对,但必须有一个,不能沉默**。 +- **零值可用性**:导出类型是否零值可用?零值不可用时(必填字段、map/slice 未初始化),godoc 有没有写明「必须用构造函数」?两个包要一致——一个提供 `NewX()`、另一个靠零值,是调用方的困惑来源。 +- **单位一致性**:同一个类型上,入口参数与出口返回值的单位是否一致(一个收「元」、一个返「分」)?把上游算出的总额直接传进去,会不会差一个数量级? ## 并发 / 服务 / 守护进程 diff --git a/references/verification.md b/references/verification.md index ca262ce..dbf5b71 100644 --- a/references/verification.md +++ b/references/verification.md @@ -13,7 +13,7 @@ | 缺参数/缺省有歧义 | 哪个现有或可预测的调用方会因此传错或绕开? | 删 | | 实现 bug | 输入/状态 → 走到哪行 → 得到什么错误结果? | 降级或删 | | 并发问题 | 两个执行体在哪个窗口交错?共享对象是什么?可见结果是什么? | 降为「待确认」 | -| 硬编码 | 它必须随什么变化?不变时会怎样静默过期? | 删 | +| 硬编码 | 它是协议固定值还是业务政策值?若是业务政策值:它必须随什么业务口径变化?不改会怎样静默过期? | 协议固定值 → 删;业务政策值且说不清随什么变化 → 删 | | 重复 | 两处符号是什么?哪条需求变化时它们必须一起改? | 降为 P3 提示或删 | | 文档不清 | 读者会因此做错什么? | 删 | | 过度设计 | 删掉它,今天的损失是什么? | 删 | @@ -40,7 +40,7 @@ - 缺参数:调用方 ___ 会因此 ___。 - 实现:输入 ___ → 执行到 ___ → 得到 ___(期望 ___)。 - 并发:执行体 A 在 ___ 与 B 在 ___ 交错;共享对象是 ___;结果是 ___。 -- 硬编码:它必须随 ___ 变化;不变时会 ___。 +- 硬编码:它是协议固定值还是业务政策值?若业务政策值:必须随 ___ 变化;不改时会 ___。 - 重复:符号 ___ 与 ___;需求 ___ 变化时两处必须一起改。 - 文档:读者会因此 ___。 - 过度设计:删掉后今天损失 ___(答不上来即成立)。 diff --git a/tools/lint-skill.mjs b/tools/lint-skill.mjs index 7e98007..0a7277d 100644 --- a/tools/lint-skill.mjs +++ b/tools/lint-skill.mjs @@ -17,6 +17,11 @@ import { join, basename, resolve } from 'node:path'; const FORBIDDEN_FRONTMATTER_KEYS = ['allowed-tools', 'license', 'model']; const MAX_BODY_LINES = 500; +// 成品里不出现相对表述:提一次「内置」,这个 skill 就始终是相对于别的东西定义的。 +// 这条规矩以前靠人工 grep 守,容易漏——机器化之后它就退不回去了。 +const RELATIVE_PHRASES = ['内置', '别的工具', '第二个工具']; +const RELATIVE_PHRASE_SCOPE = ['SKILL.md', 'README.md', 'references', 'evals']; + let errors = 0; let warnings = 0; @@ -187,6 +192,43 @@ if (existsSync(refDir)) { } } +// --- 相对表述 --- +// skill 必须自述能力,不靠对比定义。跨工具的选型说明属于「给人看的」,但也不该用 +// 「内置 X」这种措辞——正确做法是让本 skill 覆盖那件事,而不是比较谁更强。 +function collectFiles(rel) { + const abs = join(dir, rel); + if (!existsSync(abs)) return []; + return readdirSync(abs) + .filter((f) => f.endsWith('.md')) + .map((f) => join(abs, f)); +} + +const phraseTargets = [ + ...(existsSync(join(dir, 'SKILL.md')) ? [join(dir, 'SKILL.md')] : []), + ...(existsSync(join(dir, 'README.md')) ? [join(dir, 'README.md')] : []), + ...collectFiles('references'), + ...collectFiles('evals'), +]; + +let phraseHits = 0; +for (const f of phraseTargets) { + const lines = readText(f).split('\n'); + lines.forEach((line, i) => { + for (const phrase of RELATIVE_PHRASES) { + if (line.includes(phrase)) { + fail( + `${f.replace(dir + '/', '')}:${i + 1} 含相对表述「${phrase}」。` + + '成品要自述能力,不要靠与别的东西比较来定义——改写成本 skill 自己的说法。', + ); + phraseHits += 1; + } + } + }); +} +if (phraseHits === 0) { + ok(`no relative phrasing (${RELATIVE_PHRASES.join(' / ')})`); +} + console.log(''); if (errors > 0) { console.error(`✗ Lint failed: ${errors} error(s), ${warnings} warning(s)`);