HomeAgent/docs/en/engineering-lessons.md

Engineering lessons — criteria shape

Status: translation of the criteria-shape sections. Source of truth: docs/zh/engineering-lessons.md. Sections §1 (criteria that lie) and §5 (criteria shape) are translated in full; §2–§4 are summarized at the end. If the two disagree, the Chinese version wins — and this file should be corrected.

Why this document is worth the effort: the failure shapes below recur. They are more durable than any particular implementation, and each one cost a production incident to learn.

---

§1 Criteria that lie

1.1 Structural assertions are not quality assertions

Symptom: 16 criteria, 6 mutations, all green — and the feature was completely unusable.

The case: a memory-retrieval rewrite. The criteria checked "field is non-empty", "row count is right", "does not panic". Actual retrieval results:

query "how is doc-layer recall designed"  → score=0.7834
query "hmap plugin install verification"  → score=0.7922
query "QQ avatar"                         → score=0.7601

Every score landed in 0.75–0.79. Discrimination ≈ 0; the returned content had nothing to do with the query.

⇒ "All green" proved only that I implemented what I thought I implemented.

Ask: would this criterion go green if the feature were completely dead? If yes, it is not a quality criterion.

1.2 Testing "after wiring" ≠ testing "is it wired"

The most frequent defect shape in this repo (5 instances so far).

Criterion	What it does	Why it is always green
security_toolgate_test.go	calls a.SetSecurity(rt) manually, then asserts interception	the criterion supplies the missing wiring itself
retention_test.go	writes month_2026_01.tar.gz by hand, then asserts deletion	never ran compressMonthly's real output (which produces month_2026-27 — parsing necessarily fails)

Both green. Both features dead in production.

Ask: would this criterion still pass under the production assembly?

1.3 "Verification passed" ≠ "architecture is right"

16 end-to-end triple-extraction checks passed. But they verified extraction quality (can a sentence be split into subject-predicate-object), not the correct storage shape. The whole approach was rejected after the verification.

Ask: am I verifying the thing I actually want to guarantee?

1.4 Fixtures invented from imagined fields

The fixture used hand-written field names that did not match the real struct, so the criterion tested a contract that never existed.

⇒ Criterion inputs must come from real artifacts, not from memory.

1.5 Writing tests after changing the code

Changed the code first, added tests second. The tests then only proved "I changed what I thought I changed".

---

§5 Criteria shape

§1–§4 describe how criteria lie. §5 describes why a criterion's shape can be wrong — which is more expensive, because a mis-shaped criterion freezes an implementation detail into a contract. Refactoring the internals then necessarily breaks the criterion, and the author must choose: don't change the code, or delete the criterion.

5.1 A criterion must treat the module as a black box

The point of modularization is that internal changes do not break external interfaces. A criterion that does not respect that layer cancels the benefit.

White-box criteria turn internal state into contract. Measured costs:

White-box criterion	What it froze	Consequence
if sf.Step != StepLLM	which state-machine step it stops at	renaming or splitting StepLLM turns it red while behavior is correct
if f.ToolIdx == 3	how the loop advances	switching to batch-commit turns it red; the result is identical
strings.Contains(src, "kstate.Mount(ks)")	a source literal	rewriting as kstate.Mount(state) turns it red (that rewrite is correct)
os.ReadFile("../../events/bus.go") + regex	which file a constant lives in	moving the constant to types.go turns it red

Ask: does this criterion constrain an external promise, or how it happens to be implemented today? If the latter, it should not exist as a criterion — it belongs in a code comment explaining why the code is shaped that way.

Three legitimate exceptions

1. Pure algorithms — input/output is the whole semantics (trimming, version comparison, pagination). White-box here does not freeze "flow".
2. Concurrency / lifetime invariants — timing constraints invisible from outside, e.g. "emit must not read a released pointer during Dispose". These must be pinned with -race plus concurrent pressure, not by asserting internal fields.
3. Circuit-breaker / retry state machines — the internal state is itself required by the spec ("3 crashes ⇒ open"). Assert the observable effect of the transition (is it still healthy? did it enter the reload queue?), not the field value.

Operable test

"If I rewrote this module's internals completely, keeping the public interface identical, would this criterion still be green?"

Green ⇒ shape is right. Red ⇒ it pins the implementation, not the contract.

5.2 Do not assert on source text

os.ReadFile("*.go") + strings.Contains / regexp is an anti-pattern, even when the motive is sound (e.g. "prevent someone from deleting the wiring line").

Why it looks attractive: reading source is cheap compared to building a heavy fixture.

Why it must be banned:

• It tests text, not semantics. Adding a space, renaming a variable, or an equivalent refactor breaks the check (false red or false green).
• It makes file layout part of the contract. Moving code to a new file turns it red.
• It cannot distinguish "the code exists" from "the code runs".

Correct alternatives, in priority order:

What you want to guarantee	Wrong	Right
production assembly wires something	grep for kstate.Mount(ks)	start the real assembly from zero, assert an observable result (kstate.Get() != nil)
both protocol ends agree on fields	regex-extract string literals on both sides	start the handler, send a real event, assert the received JSON
a guard was not deleted	grep the source	construct the trigger condition, assert the guard fires (request rejected)
nobody deletes a criterion	grep the test file for a function name	CI-level guard: git diff --name-status detects the deletion

Only exception: CI configuration self-checks (e.g. "the exclusion table must not silently degrade"). There the object under test is the config file itself and it has no other observable behavior. Even then, the comment must say so explicitly.

5.2.1 A criterion must not cite documentation

"Tests citing docs" is wrong on its face — docs can be edited, deleted, rewritten, while criteria must be anchored to current code behavior.

Real counter-example (internal/agent/core/interrupt_invariant_test.go:336):

// Fact list (from probing, see docs/zh/interrupt-subsystem-split-plan.md §1.5).
// This only asserts "these fields exist on scheduler", as an anchor for the rewrite;

Three layers of wrong:

1. The cited document is an unimplemented plan — it is itself labeled "status: plan (not implemented)". The criterion is anchored to a fact that does not exist.
2. It tests no contract. The actual assertion is "after enqueueing, the queue has 1 item" — the most basic correctness of interrupt handling, unrelated to the claimed "reverse-dependency snapshot".
3. It turns undone work into a passing criterion. Nothing can turn it red except a total failure of the enqueue function — at which point other criteria already fail.

⇒ The right move is to delete it (or rewrite it into a real criterion) and record the TODO in future-plan.md. There is no legitimate shape called "a permanently green criterion holding a TODO".

Ask: is this criterion's basis code behavior, or a document that may change? If the latter, it protects prose, not the system.

5.3 Separate test files from business code

Measured state (2026-11-04):

test files repo-wide        521
test lines of code          112,068
internal/agent/core/        196 files, 144 of them tests (73%)
internal/plugins/webui/     29 test files, all `package webui` (white-box)
repo-wide `package X_test`  only 3

The problem is not "a lot of tests". It is:

1. ls internal/agent/core/ does not show what the module consists of.
2. 100% white-box ⇒ 100% of criteria can touch internals.
3. Converting to black-box requires moving files — and the source-scanning criteria block the move.

Convention (mandatory for new code, migrate existing incrementally):

• Black-box criteria use package <pkg>_test, in *_contract_test.go.
• White-box criteria (only the three exceptions in §5.1) go in *_internal_test.go with a header explaining why white-box is unavoidable.
• New criteria default to black-box. Writing white-box requires answering in the file header: can this be expressed black-box? Why not?

This is not a style preference: if 90% of a package's criteria are white-box, that package has no module boundary in practice — nobody dares change its internals.

5.3.1 Three real limits on test placement (measured)

After writing §5.3 I acted on it and hit three cases that cannot be migrated. Recording them so nobody (including me) assumes "all tests can move to test/".

Works: a <pkg>/test/ subdirectory

Go allows pkg and pkg_test in one directory, and allows <pkg>/test/ as a separate package. Key fact: writing package pkg inside pkg/test/ still cannot reach unexported symbols —

// pkg/test/a_test.go
package pkg
func TestX(t *testing.T) { _ = internalVar }   // undefined: internalVar

Go partitions packages by directory. So a subdirectory and an external package are equivalent in terms of visibility: both force black-box.

Cannot migrate 1: package main (CLI)

import "x/cmd" is a program, not an importable package

tools/hmapdev (19 test files), cmd/homed, etc. must stay in-package.

Cannot migrate 2: tests that construct private fields

SDK tests wrote &PluginSDK{regStage: regStage, name: "test"} — after migration they cannot even construct the object under test (cannot refer to unexported field regStage).

But this does not mean white-box is required: New(name, sett, regTool, regStage, regAPI, regOutput) already takes these as parameters. It is equivalent and it is the production path. All 19 instances were convertible, and the criteria still caught mutations afterwards.

⇒ When you "need to set a private field", first ask: is there an exported constructor that already accepts it? Most of the time there is; the test was just taking a shortcut.

Cannot migrate 3: criteria that scan "the current directory's source"

files, _ := filepath.Glob("*.go")     // childkind_test.go

After moving to a subdirectory this globs an empty directory, and the criterion silently loses its subject — no compile error, just green. Convert these to a fixed relative path, or rewrite per §5.2.

Migration order: one file at a time, let the compiler talk

My first attempt migrated 11 files in bulk and then ran a script to prepend core. to identifiers. The script also rewrote import "…/internal/agent/core" itself (producing core.core.core) and clobbered strconv.Itoa.

Text substitution is unreliable for "prepend a package qualifier" — it hits import paths, comments, and standard-library symbols. Migrate one file, read the compiler error, fix by hand. Slow, but it leaves no half-migrated state (agent/core had to be fully rolled back once because of this).

5.3.2 Coverage requires -coverpkg=./...

After moving criteria to <pkg>/test/, the default coverage accounting loses them:

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

It only counts coverage measured from tests inside each package. But the tests covering child live in child/test/ — a different package. So the main report shows:

internal/agent/core/child        0.0%     ← wrong
internal/agent/core/child/test   87.6%    ← actually covering child

With -coverpkg=./...:

internal/agent/core/child       85.7%
global                           56.9% → 63.0%

The earlier number was a systematic underestimate. Under-reporting is more dangerous than over-reporting: it pushes people to "write empty criteria to raise the number", which is exactly the trap in §1.1.

⇒ Separating criteria (§5.3) and collecting coverage must change together. Change only the former and the metric degrades into a lie.

5.4 CI must cover every independent module

Measured gap (2026-11-04): the repo contains third_party/hologramagentsdk/, which has its own go.mod and its own .git (remote: github.com/JianFeeeee/hologramagentsdk).

Consequence:

go list ./... | grep third_party   →  0

The main repo's go test ./... never includes any SDK test.

• The SDK repo's own CI does run them (its .github/workflows/ci.yml has go test ./... and separately handles the nested module)
• But the main repo's CI only verifies "it compiles", not "the SDK's criteria still pass"

⇒ A main-repo change may depend on SDK internals, and the main repo will not notice when SDK criteria go red.

Rules:

1. A go.mod in a subdirectory ⇒ it is an independent module, invisible to go test ./...
2. Every independent module needs its own test step in CI
3. A directory with its own .git is not managed by the main repo; the main repo only vendors what it needs. When go.mod replaces it, CI must ensure those files exist and match upstream HEAD

5.5 CI and local must run the same gates

Measured gap: the commit discipline requires gofmt -l to be clean, but gofmt appeared 0 times across three workflows. The rule was enforced only by human diligence. Same for staticcheck / govulncheck / coverage.

Rule: every item written into the commit discipline must have a corresponding CI step. Otherwise it is a suggestion, not a discipline.

Discipline item	CI backstop
go build ./...	✅
go vet ./...	✅
gofmt -l clean	✅ (added 2026-11-04)
go test -race green	✅
clean worktree compiles	❌ human only

---

§2–§4 (summarized)

§2 Wiring. Mechanism existing ≠ being wired (codegraph_callers returning 0 is the test). The dependency graph is the only thing that guarantees init order — reading another module's field without declaring the facility dependency works only by layer coincidence. When you change the singular path, do not forget the plural one.

§3 Permissions & lifetime. "Wired up" has three levels: mechanism exists → called from the execution chain → model is correct. Tables keyed by a caller-supplied name must be cleaned up when the lifetime ends, or same-name re-creation inherits the previous generation's state. Privilege escalation must be derived by the security module from facts, never declared by the caller.

§4 Process. Never run memory-heavy exports concurrently with tests. Numeric claims in docs must be produced by a command, not recalled from memory. Plans must not enter the repository — after completion they read as "what we intended" while being read as "what is".

---

Appendix: the common shape

There is always a gap between a signal that looks right and actually being right.

• criteria all green ≠ feature usable
• mechanism exists ≠ wired
• layer index happens to be right ≠ dependency declared correctly
• singular path right ≠ plural path right
• same name ≠ same thing
• plan written ≠ work done
• criterion passes ≠ contract protected (it may pin the implementation, §5.1)
• go test ./... green ≠ all tests ran (independent modules are excluded, §5.4)
• discipline written down ≠ anyone enforces it (without a CI backstop it is advice, §5.5)

⇒ When writing a criterion, a document, or code, ask: under what circumstances would this signal be false?