admin
c21bcf84f7
gates / gates (push) Successful in 7s
New shared scripts/instructions_gate.py, registered in controller_gates.py and agent_gates.py, never copied into a sibling repo (the reuse_refs_check.py precedent). 20 fixture tests, all asserting the effect: exit code AND that the message names the file and the reason. It is a consistency gate, not a budget gate, and the failure message says so. A /context reading measured the instruction files at 15k tokens against 869k free in a 1M window -- space is not the constraint, and a future reader must not re-derive the wrong reason. The 200-line ceiling is adherence guidance; a file nobody can hold in their head is where contradictions hide, and five were found here. Checks run against effective text (HTML comments stripped, because they are stripped before injection): the line ceiling; every .claude/rules/*.md declares paths: or an explicit unconditional: true; no component version literal; no TEMPORARY block carrying a past date; and the workspace-root CLAUDE.md is byte-identical to its versioned copy -- the live file sits outside any git repo, so that copy is its only version-controlled record. Two traps recorded so they are not reintroduced: a bare \d+\.\d+\.\d+ matches the first three octets of every IPv4 (the gate excludes dotted quads, or it fails on 192.168.0.180 in the agent's own file); and unconditional: true is NOT a Claude Code feature but this project's own marker. Workspace-root CLAUDE.md 208 -> 182 lines (142 effective), copy kept identical. The nine-instance invariant table moved into the felhom-testing skill, which triggers when writing or reviewing a test; all three directive bullets stayed in the core. felhom.eu/CLAUDE.md got surgical corrections only and is knowingly still over the ceiling at 227 effective lines -- closing it needs the restructure R-229 defers, said plainly rather than quietly absorbed. CONTEXT.md gains standing ruling S-35. OPEN-ITEMS.md gains R-229. Docs only -- no Go, no version bump, nothing built or deployed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JJc8sAGRWmavP3rMtdpkr2
100 lines
7.8 KiB
Markdown
100 lines
7.8 KiB
Markdown
---
|
||
name: felhom-testing
|
||
description: Felhom testing doctrine — use when writing or reviewing ANY Go test in felhom-controller, felhom-agent, or the felhom.eu hub, and for EVERY correctness or security fix (the red-proof is mandatory there). Triggers - "write a test", "add tests", reviewing a diff that changes logic, fixing a bug, hardening a guard, or validating a fix live. Contains the non-hollow rules, the companion red-proof procedure, seam locations, and the green-gate command.
|
||
---
|
||
|
||
# Felhom testing doctrine
|
||
|
||
## Non-hollow rule (the cardinal one)
|
||
|
||
A test must assert the **effect**, not the absence of error. Wrong: HTTP 200 came back, `err == nil`,
|
||
"function ran". Right: the stored row has the expected value, the rendered HTML contains the badge,
|
||
the state file transitioned, the fake recorded the exact command. If deleting the fix wouldn't fail
|
||
the test, the test is hollow.
|
||
|
||
## Companion red-proof (mandatory for every correctness/security fix)
|
||
|
||
Prove the test detects the bug it guards against:
|
||
|
||
1. Temporarily restore the PRE-FIX shape (revert the fixed line, or model the old predicate inline).
|
||
2. Run the test → it must **FAIL**, with the wrong value visible in the failure message.
|
||
3. Restore the fix → test passes. `git diff` clean.
|
||
4. Record the red-proof outcome in `REPORT.md` (what failed, with what value).
|
||
|
||
In-tree exemplars (verified):
|
||
- `felhom.eu/hub/internal/notify/dispatcher_test.go` `TestSeverityNotifies` (~L27–49) — models the
|
||
pre-fix `warning||error` predicate inline and asserts the fix routes what it dropped.
|
||
- `felhom.eu/hub/internal/api/event_test.go` `TestHandleEvent_CriticalPreserved` — asserts the STORED
|
||
severity; its red-proof was run by reverting the one-line switch (stored `"info"` → FAIL).
|
||
|
||
## Seams over shell-outs
|
||
|
||
Never let a unit test touch docker/pct/real /dev. Every repo's **`REUSE.md` §4** lists its seams and
|
||
existing fakes — inject there:
|
||
- controller: `diskAgent` (`mockAgent`), `quiesce.Backend/Stacks`, `channelhealth.Probe/Sink`,
|
||
`selfupdate.AgentSwapper`, `offboxRunner`, `bootstrap.PullFunc`.
|
||
- agent: `proxmox.Runner` (`mockRunner`), `storage.HostOps/HostReader`, localapi `Options` fakes,
|
||
Server seam funcs (`reresolveWipe`, `deviceDurableID` — override in tests, no real /dev).
|
||
- hub: `Dispatcher.sendEmailFn`, `mailrelay.Sender`, `mailRateLimiter.now` (clock),
|
||
`monitor.EventNotifyFunc`, provider interfaces on the api Handler.
|
||
|
||
Test harness conventions: real store on `t.TempDir()` DB (`hub/internal/api/host_test.go`
|
||
`newTestHandler` pattern); `t.Cleanup` for teardown; table-driven where natural; pure
|
||
classifier functions get fixture tables (agent `classifyClaim` style).
|
||
|
||
## What every test suite should also cover
|
||
|
||
- **Negative cases:** the 400/401/403/refusal paths, not just the happy path (e.g. unknown
|
||
event_type → 400 AND nothing stored).
|
||
- **Idempotency:** re-running the op is a clean no-op where the contract says so (registry add,
|
||
intent set, mount ensure).
|
||
- **Fail-safe direction:** for guards, ambiguity must refuse (agent claim/role classifiers are the
|
||
canon — any read error ⇒ most-protected verdict).
|
||
|
||
## Green gate (run before every commit that touches Go code)
|
||
|
||
```bash
|
||
go build ./... && go vet ./... && go test ./...
|
||
```
|
||
Run it in the module dir: `felhom.eu/hub/`, `felhom-controller/controller/`, `felhom-agent/` root.
|
||
**No known flakes.** The long-standing agent `TestGenerateRecoveryCode_EntropyAndFormat` ~1/5 failure
|
||
was **fixed in agent v0.93.0 (2026-07-21)** — and it was never a flaky test. It was a real defect the
|
||
test was correctly detecting and everyone had been told to re-run past: the EFF wordlist contains four
|
||
hyphenated entries, so a recovery code could come out reading as 11 words instead of 10. If it fails
|
||
now, it is a regression. **The lesson generalises: "known flake, just re-run it" is a diagnosis, and
|
||
it needs the same evidence as any other one.** A test that fails at a stable, explainable rate is
|
||
usually telling the truth about a rare input, not misbehaving.
|
||
|
||
## A comment asserting an invariant needs a test pinning it — the nine instances
|
||
|
||
The three directives live in the workspace-root `CLAUDE.md` and apply always. This is the evidence
|
||
behind them: **nine shipped guarantees the code did not provide**, each surviving review because the
|
||
comment read as settled. Read this table when you are about to trust a comment, or write one.
|
||
|
||
| # | Comment | What it claimed | What the code did |
|
||
|---|---|---|---|
|
||
| 1 | `EffectiveProtected` | a stack was protected | it was not — the samba false alarm |
|
||
| 2 | `newestArchiveOn` | *"errors degrade to unknown, never to no-backup"* | the `(time,bool)` signature made that impossible (R-88 Part 2) |
|
||
| 3 | R-97a operator-only | the event *"cannot be routed to a customer"* | only configuration stopped it; fixed by a real `operatorOnlyEvents` register |
|
||
| 4 | `classifyRunStates` I1 | *"StateStopped means deliberately stopped by the user"* | quiesce stops stacks the same way — a failed restart was silent (F-CRIT-1) |
|
||
| 5 | `inflight.go` | *"a caller that cannot acquire DEFERS"* | the backup caller recorded a failure and paged the operator (F-A1) |
|
||
| 6 | `quiesce.go` | the agent's 409 *prevents* "a spurious failure" | on the start path it produced one (F-A1) |
|
||
| 7 | `recovery_unit.go` B2 refusal (R-181) | *"the previous unit is untouched and NOTHING was deleted"* | *nothing deleted* held; **untouched was measured false** — the floor was checked ONLY in `captureAllRecoveryUnits`, while the two dump legs wrote the bulk into the same tree first and unguarded, so a 182,272 B tar became 2,147,666,432 B under a manifest that had not moved |
|
||
| 8 | `ResolveManagedFloor` (R-216) | *"never push a controller past the agent it depends on"* | it compared the box's agent against the **golden's** MinAgent while serving a **floor** that could point elsewhere. Measured live 2026-08-05: golden 0.192.0/MinAgent 0.113.0, floor 0.200.0, agent 0.120.0 → served, and the box landed on a controller needing 0.125.0. Its customer was then told their correct recovery code was wrong. **The first entry where the false invariant was a GUARD, not a comment alone.** Fixed hub v0.97.0 |
|
||
| 9 | `escrow/recover.go` header (R-224) | *"The errors below are DISTINCT on purpose"*, naming **three** situations | there were **four**. A failed FETCH fell through the local-api handler's `default` into the wrong-code answer, so an unreachable hub was reported to the customer as a bad recovery code. Measured live 2026-08-05: **0.0556 s** hub-firewalled and **0.0299 s** agent-stopped, against ~1.0 s for a genuine unseal. **AND A GREEN TEST NAMED IT AND DID NOT PREVENT IT** — `TestRecoverOffsiteRepoPassword_FetchErrorIsDistinct` asserted the package's error **string**, one layer below where the merge happened, and a string is not something a caller can branch on. **Mechanism asserted, consequence unpinned.** Fixed agent v0.126.0 + controller v0.202.0 |
|
||
|
||
**Three of these (4, 5/6 and 7) were found on live hardware**, not by review or unit tests. #4 had a
|
||
green, red-proofed suite over a production path broken two independent ways. #7 survived a full green
|
||
suite plus three of its own red-proofs, because every one asserted the mechanism inside
|
||
`captureAllRecoveryUnits` and none asserted the **consequence** across the whole backup run. The test
|
||
that would have caught it is the one #7's fix ships: **fingerprint the tree before and after, and
|
||
compare.**
|
||
|
||
## Live validation doctrine (after unit-land)
|
||
|
||
Exercise the SERVER-SIDE PIPELINE a real user triggers, end-to-end — never hand-set state around it
|
||
(the F9 lesson). Invoking the exact endpoint the UI invokes is an acceptable proxy when a browser
|
||
isn't available; the residual is client-side rendering only — SAY which method was used. Test crash
|
||
behavior with kill -9 / OOM, never `docker kill` (containers' restart policy masks the difference).
|
||
Don't echo API keys/tokens into logs or REPORT — extract into shell vars, print lengths only.
|