Files
HomeAgent/docs/zh/engineering-lessons.md
JianFeeeee 77e38360a8 ci+docs: 覆盖率改用 -coverpkg=./... —— 默认统计会漏掉外部测试包
## 缺陷(实测发现,且是**度量本身**在说谎)

加覆盖率采集之后,`internal/agent/core/child` 报告 0.0% —— 但刚给
它写了 9 条判据、覆盖 87.6%。

原因:`go test ./... -coverprofile` **只统计每个包内自测的覆盖**。
而覆盖 child 的测试在 `child/test/` —— 另一个包。主报告里只看到:

    internal/agent/core/child        0.0%     ← 假的
    internal/agent/core/child/test   87.6%    ← 真的在覆盖 child

## 修法

    go test ./... -coverpkg=./... -coverprofile=/tmp/cover.out

实测差异:

    child               0.0%  → 85.7%
    全局总覆盖率        56.9%  → 63.0%

即**此前的数字是系统性低估的**。而低估比高估更危险 —— 它会促使人
"为凑覆盖率写空判据",那正是 engineering-lessons §1.1 记的坑。

代价:`-coverpkg=./...` 为每个测试二进制插桩全部包,跑得更慢
(实测仍在 20m 门内)。这个代价换"数字是真的",值得。

## 文档补 §5.3.2

把这条写进 engineering-lessons,因为它是**判据分离的连带义务**:

> 判据分离(§5.3)与覆盖率采集必须同时改。
> 只改前者,度量会退化成谎报。

否则下次有人把测试迁到 test/ 之后,会看到覆盖率"莫名其妙掉了",
进而怀疑分离本身,把它改回去。

## 本提交同时包含

`internal/agent/core/child/test/registry_contract_test.go`(9 条判据)
已随上一个提交(8884cf9)入库 —— 那里和 io 的并发判据修复放在一起,
因为它们都是"判据自身的缺陷"这一主题。
2026-11-04 19:27:01 +08:00

450 lines
18 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# 工程教训
> 这个仓库踩过的坑,以及它们共同的形态。**写在这里是因为它们比实现细节更耐用** ——
> 代码会改,缺陷形态会重复出现。
>
> 来源:这些条目原本散落在各份执行计划(`memory-*-plan.md`、
> `toolcall-parallel-execution-plan.md` 等)里。计划本身已随实施完结而删除,
> 但教训被提出来了 —— 那是它们唯一还有价值的部分。
>
> 建立于 2026-10-31。
---
## 一、判据类(最贵的教训,占了一半)
### 1.1 「结构判据」不能替代「质量判据」
**症状**:16 条判据、6 个变异**全部通过**,而功能完全不可用。
**当时的场景**:记忆检索改造。判据验的是「字段非空」「条数正确」「不 panic」,
而实际检索结果是:
```text
查询 "doc 层召回是怎么设计的" → score=0.7834
查询 "hmap 插件安装验签" → score=0.7922
查询 "QQ 头像" → score=0.7601
```
**分数全挤在 0.75~0.79,区分度 ≈ 0,命中内容与查询毫无关系。**
⇒ 「全绿」证明的只是**我实现了我以为的东西**,不是它可用。
**判据要问**:这条判据在"功能完全失效"时会绿吗?会绿就不是质量判据。
### 1.2 判据测「接线之后」≠ 测「有没有接线」
这是本仓出现频率最高的缺陷形态(已攒够 5 例,见 `future-plan.md` §〇)。
两处实测:
| 判据 | 它做了什么 | 为什么永远绿 |
| --- | --- | --- |
| `security_toolgate_test.go` | 手动调 `a.SetSecurity(rt)` 再断言拦截 | **判据自己把缺失的接线补上了** |
| `retention_test.go` | 手写 `month_2026-01.tar.gz` 再断言被删 | 从没跑过 `compressMonthly` 的**真实产物**(那产出的是 `month_2026-27`,解析必失败) |
两者都全绿,两个功能在生产里都完全没生效。
**判据要问**:**这条判据在生产装配下也会绿吗?**
### 1.3 「验证通过」≠「架构正确」
**当时的场景**:`630d3f4` 的 16 三元组端到端验证全部通过。
但那次验证的是**拆分质量**(能不能把句子拆成主谓宾),
不是**正确的落库形态**(拆出来的三元组该以什么结构存)。
⇒ 验证通过之后才发现,整个蒸馏路线方向错了,要整体否决。
**判据要问**:我验证的,是不是我真正想保证的那件事?
### 1.4 fixture 照臆测字段编
本仓 T23 教训:判据用的 fixture 是
**手写臆测的字段名**,而真实结构体字段不同 ⇒ 判据测的是一个不存在的契约。
⇒ 判据的输入应当来自**真实产物**,不是人脑里的想象。
### 1.5 先改代码再写测试
原本记在记忆块流修复计划里(该计划已随实施完结删除):先改代码再补测试,
测试就只会证明「我改的是我以为的」。
⇒ 这个顺序本身会让判据失去独立价值。
---
## 二、接线类
### 2.1 机制 ≠ 接线
见 `future-plan.md` §〇。一句话总结:
**策略表存在、判定函数正确、日志里一条拒绝记录都没有** —— 看起来像「没人违规」。
判断方法:`codegraph_callers(符号名)`。0 个生产调用方就是没接线。
### 2.2 依赖图是唯一保证顺序的地方
`agent` 模块读了 `ks.Security` 却没在 `Requires()` 里声明 `FacSecurity`。
「恰好」能用(security 在第 1 层、agent 在第 4 层),但那是**偶然**:
一旦 security 加了自己的依赖被推后,就会静默拿到 nil。
⇒ 读了谁的字段,就必须声明对谁的依赖。**不要靠层号碰巧对。**
### 2.3 单数路径改了,复数路径漏了
`StopRTChild`(单数)注销入站登记,`StopRTChildren`(复数)漏了。
残留随 create/destroy 单调累积。
⇒ 写这类对称函数时,判据要**同时**钉两条路径。本仓已有判据专门做这件事。
---
## 三、权限与生命周期类
### 3.1 「接上了」有三种层次,容易停在第二层
| 层次 | 内容 |
| --- | --- |
| 1 | 机制存在(`CheckTool` / `ScanContent` 都写了) |
| 2 | 接到了执行链(`securityDecisionFor` 真的调它) |
| 3 | **模型正确** |
第三层的典型错误:把「子 agent 继承父的权限」当成「父为子声明策略」。
两者都"接上了",但权限模型完全不同 —— 前者意味着子等于父,后者意味着子更窄。
**判断标准**:**这条策略是谁的意图?** 权限的意图只能来自委派者(父),
不能来自被委派者(子),也不能来自内核默认(全局)。
### 3.2 以「调用方给的名字」为键的表,必须在生命周期结束时清理
两个实例:
- `rtClosedTranscripts[id]` —— 以模型给的名字为键,全仓 0 处删除。
结果:同名重建的 rtchild 读到上一通电话的原文
- `scopesByAgent[agentID]` 的 `longtimeProfile` —— 不释放则同名重建的 agent
继承上一代的约束
⇒ 键来自外部(模型、调用方)就不能假设它唯一。
### 3.3 提权口子:让调用方声明自己是谁
`Kind: "resident"` 由 agent 自己写 —— 它同时意味着「我是内部主体」
(豁免 HardPrivateTools)和「我不受收窄」。调用方说自己是内部就拿到豁免。
⇒ 类别必须由安全模块从**事实**(谁在调、有没有父)推出,不能由调用方声明。
---
## 四、流程类
### 4.1 长任务纪律
导出、回填等内存密集任务**绝不与测试并行**(同一会话里踩了三次 OOM)。
### 4.2 数字类断言要现场取,不要凭记忆
文档里写「17 模块 6 层」,实际是 **20 模块**。给读者的是错的坐标。
⇒ 数字必须附取数命令(如 `go run ./cmd/homed-depgraph -print`)。
### 4.3 计划文档不要进仓库
执行计划会在实施完结后变成**误导性读物**:它记录的是"当时打算做什么",
而读者会读成"现在是什么"。
本仓删掉了 9 份已完结的计划(`memory-*-plan.md` 等),保留:
- **设计文档**(`*-design.md`):为什么这样设计,有长期价值
- **教训**(本文):踩过什么坑
- **未做登记**(`future-plan.md`):还剩什么没做
- **操作手册**(`*-runbook.md`):怎么做
---
## 五、判据形态类(2026-11-04 立)
> 这一节回答一个具体问题:**判据应该写成什么形状?**
>
> 前面四节讲"判据会骗人"的各种形态。这一节讲**判据本身的形状错了**会怎样 ——
> 那比"判据写得不够严"更贵,因为形状错的判据会把**实现细节固化成契约**,
> 于是重构内部就必然破坏判据,作者只能二选一:不改代码,或者删判据。
### 5.1 判据必须把被测模块当黑箱
**模块化的目的就是"改内部实现不破坏外部接口"。判据如果不按这个层次写,
就等于取消了模块化的收益。**
白盒判据把内部状态变成契约。实测代价:
| 白盒判据 | 它固化了什么 | 后果 |
| --- | --- | --- |
| `if sf.Step != StepLLM` | 状态机停在哪一步 | 把 `StepLLM` 改名/拆步骤就假红,而行为完全正确 |
| `if f.ToolIdx == 3` | 循环推进方式 | 改成"整批提交"就假红,而结果一样 |
| `strings.Contains(src, "kstate.Mount(ks)")` | 源码字面量 | 改写成 `kstate.Mount(state)` 就假红(那是对的) |
| `os.ReadFile("../../events/bus.go")` 正则取常量 | 常量定义在哪个文件 | 把常量挪到 `types.go` 就假红 |
**判据要问**:**这条判据约束的是"对外承诺",还是"当前怎么实现的"?**
如果是后者,它就不该存在 —— 或者必须降级为"实现注释"(代码里的注释说明
为什么这样做),而不是判据。
#### 允许白盒的三种例外(其余一律黑箱)
1. **纯算法**:输入输出就是全部语义(裁剪算法、版本比较、分页计算)。
这类即使白盒也不固化"流程",因为它本来就只有"输入→输出"。
2. **并发/生命周期不变量**:无法从外部观测的时序约束,如"Dispose 期间
emit 不能读到已释放的指针"。这类**必须**用 `-race` + 并发压力,
而不是断言内部字段。
3. **熔断/重试状态机**:内部状态本身就是被需求规定的("连续 3 次崩溃
进入熔断")。这类要断言**状态转移的可观测效果**(还健康吗、
进重载队列了吗),不是断言状态字段的值。
#### 一个可操作的判据
写完一条判据,问自己:
> **"如果我把这个模块的内部实现完全重写、公开接口不变,
> 这条判据还会绿吗?"**
- 会绿 → 形状对了
- 会红 → 它钉的是实现,不是契约
### 5.2 禁止"读源码做断言"
`os.ReadFile("*.go")` + `strings.Contains` / `regexp` 是**反模式**,
即使动机正确(如"防止有人删掉装配的那一行")。
**为什么它看起来有吸引力**:当行为判据需要很重的 fixture 时,读源码很便宜。
**为什么它必须禁用**:
- 它测的是**文本**,不是**语义**。加空格、换变量名、等价重构都会让判断失效
(假红或假绿)
- 它把"文件布局"变成契约。移动代码到新文件就假红
- 它无法区分"代码存在"与"代码被执行"
**正确的替代**(按优先级):
| 你想保证的 | 错误做法 | 正确做法 |
| --- | --- | --- |
| 生产装配接了某根线 | 读源码找 `kstate.Mount(ks)` | 从零启动真实装配,断言**可观测结果**(如 `kstate.Get() != nil`) |
| 协议两端字段一致 | 正则提取两边的字符串字面量 | 起 handler,发真实事件,断言**收到的 JSON 字段** |
| 某段保护代码没被删 | 读源码找字符串 | 构造触发条件,断言**保护真的生效**(如请求被拒绝) |
| 某人没删掉某条判据 | 读测试文件找函数名 | CI 层守卫:`git diff --name-status` 检测删除 |
**唯一的例外**:CI 配置自检(如"排除表不许静默退化")。
那类检查的**对象就是配置文件本身**,读它是正当的 —— 因为它没有别的
可观测行为。但即便如此,也要在注释里写明"这是配置自检,不是行为判据"。
### 5.2.1 判据不得以文档为依据
**"测试引用文档"本身就是错的** —— 文档可以被改、被删、被改写,
而判据必须锚在**代码的当前行为**上。
实测反例(`internal/agent/core/interrupt_invariant_test.go:336`):
```go
// 事实清单(来自探测,见 docs/zh/interrupt-subsystem-split-plan.md §1.5)。
// 这里只断言"这些字段在 scheduler 上存在",作为改写时的锚点;
```
问题有三层:
1. **它引用的是一份"未实施计划"** —— 被引用方自己写着
「状态:计划(未实施)」。判据锚在一个**不存在的事实**上。
2. **它没测任何契约**。实际断言只有"入队后队列里 1 条",
而那是中断功能最基本的正确性 —— 跟它声称的"反向依赖现状快照"毫无关系。
3. **它把"未做的工作"变成了通过的判据**。改任何东西都不会让它红,
除非入队函数彻底坏掉;而那时别的判据早就红了。
⇒ 出现这种形态时,正确处置是**删掉它**(或改写成真判据),
然后把这个待办登记进 `future-plan.md`。
**不存在"用一条永远绿的判据占位待办"这种合法形态。**
**判据要问**:这条判据的依据是代码行为,还是一份可能会变的文档?
如果是后者,它保护的不是系统,是文字。
### 5.3 测试文件与业务代码分离
**现状(2026-11-04 实测)**:
```text
全仓测试文件 521 个
测试代码行数 112,068 行
internal/agent/core/ 196 个文件里 144 个是测试(73%)
internal/plugins/webui/ 29 个测试文件,全部 package webui(白盒)
全仓 package X_test 仅 3 个
```
问题不是"测试多",而是:
1. `ls internal/agent/core/` 看不出这个模块由什么组成
2. 100% 白盒 ⇒ 100% 的判据都能碰内部
3. 想改成黑箱必须先移动文件,而移动文件又会被源码扫描类判据挡住
**约定**(新代码必须遵守,存量逐步迁):
- **黑箱判据用 `package <pkg>_test`**,放 `*_contract_test.go`
- **白盒判据**(仅 §5.1 的三个例外)放 `*_internal_test.go`,
并在文件头注明"为什么必须白盒"
- 新增判据**默认黑箱**;要写白盒必须在文件头回答:
这条判据能用黑箱表达吗?为什么不能?
**这不是风格问题**:一个包里如果 90% 判据是白盒,这个包的模块边界
事实上不存在 —— 谁都不敢改内部实现。
### 5.3.1 三种测试位置的实际边界(2026-11-04 实测)
写完 §5.3 之后按它动手,撞到三类不可迁移的情况。这里把边界记清,
避免以后有人(包括我)以为"所有测试都能挪到 test/"。
#### 可行:`<pkg>/test/` 子目录
Go 允许同目录下 `pkg` 与 `pkg_test` 两个包名,也允许 `<pkg>/test/` 放
独立包。**关键事实**:`pkg/test/` 里写 `package pkg` 也访问不到未导出符号 ——
```go
// pkg/test/a_test.go
package pkg
func TestX(t *testing.T) { _ = internalVar } // undefined: internalVar
```
Go 按**目录**划分包实例,不同目录就是不同包。所以"子目录"和"外部包"
在可见性上等价,都强制黑箱。
#### 不可行一:`package main`(CLI)
```
import "x/cmd" is a program, not an importable package
```
`tools/hmapdev`(19 个测试文件)、`cmd/homed` 等属于这类,**必须留在包内**。
#### 不可行二:测试直接构造私有字段
SDK 原测试写 `&PluginSDK{regStage: regStage, name: "test"}` —— 迁到外部包后
连**构造被测对象**都做不到(`cannot refer to unexported field regStage`)。
**但这不等于必须白盒**:`New(name, sett, regTool, regStage, regAPI, regOutput)`
本来就把这些作为参数,等价且是生产路径。实测 19 处全部可改,改完判据仍然
咬得住变异。
⇒ 遇到"要设私有字段"时,先问:**有没有一个导出的构造入口已经接受这些参数?**
大多数情况有,只是测试偷懒没用。
#### 不可行三:判据扫描"当前目录源码"
```go
files, _ := filepath.Glob("*.go") // childkind_test.go
```
迁到子目录后 glob 到**空目录**,判据静默失去对象 —— 不报编译错,直接绿。
这类要先改成"扫描固定相对路径",或者干脆按 §5.2 重写成行为判据。
#### 迁移顺序:一次一个,让编译器说话
第一版我批量迁 11 个文件、再用脚本自动加 `core.` 前缀,结果把
`import "…/internal/agent/core"` 本身也改了(变成 `core.core.core`),
还误伤 `strconv.Itoa` 等标准库符号。
文本替换在"加包名前缀"这种场景不可靠 —— 它会改到导入路径、注释、
标准库符号。**正确做法是一次只迁一个,逐步看编译器报什么,手工修。**
慢,但不会留下半成品(agent/core 已因此完全回滚过一次)。
### 5.3.2 覆盖率必须用 `-coverpkg=./...`
把判据迁到 `<pkg>/test/` 之后,**默认的覆盖率统计会把它们算丢**:
```bash
go test ./... -coverprofile=cover.out
```
只统计"每个包内自测的覆盖"。而覆盖 `child` 的测试在 `child/test/` —— 另一个包。
于是主报告里:
```text
internal/agent/core/child 0.0% ← 假的
internal/agent/core/child/test 87.6% ← 真的在覆盖 child
```
加 `-coverpkg=./...` 之后:
```text
internal/agent/core/child 85.7%
全局 56.9% → 63.0%
```
**即此前的数字是系统性低估的。** 而低估比高估更危险 —— 它会促使人
"为凑覆盖率写空判据",而那正是 §1.1 记的坑。
⇒ 判据分离(§5.3)与覆盖率采集必须同时改。只改前者,度量会退化成谎报。
### 5.4 CI 必须覆盖每个独立 module
**实测缺口(2026-11-04)**:主仓有 `third_party/hologramagentsdk/`,
它有自己独立的 `go.mod`(模块 `github.com/JianFeeeee/hologramagentsdk`)
**和独立的 `.git`**(remote: `github.com/JianFeeeee/hologramagentsdk`)。
后果:
```text
go list ./... | grep third_party → 0 个
```
主仓的 `go test ./...` **永远不包含** SDK 的任何测试。
- SDK 仓自己的 CI 会跑(已核实它的 `.github/workflows/ci.yml` 有
`go test ./...` 且单独处理了嵌套 module)
- 但**主仓 CI 只验证"能编译"**,不验证"SDK 的判据还绿"
⇒ 主仓改动可能依赖 SDK 的内部行为,而 SDK 判据红了主仓不知道。
**规则**:
1. 子目录里出现 `go.mod` ⇒ 它是独立 module,`go test ./...` 覆盖不到
2. 每个独立 module 必须在 CI 里有**自己的**测试步骤
3. 有独立 `.git` 的目录**不由主仓管理**,主仓只 vendor 必需文件;
`go.mod` 的 `replace` 指向它时,CI 必须确保这些文件存在且与
上游 HEAD 一致
### 5.5 CI 与本地必须跑同一套门
**实测缺口**:提交纪律要求 `gofmt -l` 干净,但三个 workflow 里
`gofmt` 出现 **0 次**。纪律只靠人遵守,CI 不兜底。
同理缺 `staticcheck` / `govulncheck` / coverage。
**规则**:写进提交纪律的每一条,都必须在 CI 里有对应步骤。
否则它不是纪律,是**建议**。
| 纪律条目 | CI 有没有兜底 |
| --- | --- |
| `go build ./...` | ✅ |
| `go vet ./...` | ✅ |
| `gofmt -l` 干净 | ❌ **必须补** |
| `go test -race` 全绿 | ✅ |
| 干净 worktree 能编译 | ❌ 靠人 |
---
## 附:这些教训的共同点
回头看,几乎每一条都是同一个形态的变体:
> **某种"看起来对了"的信号,与"真的对了"之间存在缺口。**
- 判据全绿 ≠ 功能可用
- 机制存在 ≠ 接线生效
- 层号碰巧对 ≠ 依赖声明正确
- 单数路径对 ≠ 复数路径也对
- 名字相同 ≠ 是同一个东西
- 计划写完 ≠ 事情做完
- **判据通过 ≠ 契约被保护**(它可能钉的是实现,见 §5.1)
- **`go test ./...` 绿 ≠ 全部测试跑过**(独立 module 不在里面,见 §5.4)
- **纪律写着 ≠ 有人执行**(CI 没兜底的纪律只是建议,见 §5.5)
**所以写判据、写文档、写代码时,都要问一句:我现在拿到的这个信号,
在什么情况下会是假的?**