v0.128.0 — R-221: the escrow seed is asserted every tick, not remembered once

A rebuilt box could not run the escrow ceremony AT ALL, with no way forward from inside the product. This was the only open item blocking a customer from something we promise them. MECHANISM, established at file:line rather than assumed. The preflight refuses on escrow.pbs_storage_id; the pbsdr bridge writes that key; and it wrote it from exactly one place — finishConverged, reached only on the paths that actually converge. The marker and the key live in different places and die at different times. The marker is host-side (<agent-state>/pbsdr/marker.json). The key is in agent.json, which step_agent_config renders from `base = {}` unless an explicit --preserve-from is given (felhom.eu/scripts/felhom-host-install.sh: 2396 the step, :2449 the render, :2579 the O_TRUNC write; the flag :1246, defaulting empty at :256) — AND THE RENDER NEVER WRITES AN escrow SECTION AT ALL (grep over the whole heredoc: zero hits). So a rebuild keeps the marker and takes the key: same descriptor, same hash, early return, and the seed never runs again into a config that no longer has it. A rebuild is only the case that was measured. The same hole opens for a hand-edited or restored config, which is the honest reason this is a seam fix rather than an installer fix: the seed must be a thing the loop ASSERTS, not a thing it did once. Apply now re-asserts the seed BEFORE the idempotent early return. seedEscrowStorageID is unchanged and still never clobbers a different existing value — an operator's own choice outranks the descriptor's, with a warning naming both. THE EARLY RETURN IS KEPT. It stops a converged box re-running Proxmox operations every 60s, and TestSeedReasserted_OnConvergedTick_WithZeroProxmoxCalls asserts ZERO recorded runner calls on that tick, so a "fix" that simply deleted the return would fail. Cost: one small file read plus a JSON parse per tick, no exec, no network, early-returning once the value matches. A seed failure can never un-converge the box: Warn plus a message on the published status, exactly as finishConverged does it — no marker write, no state change. Tests drive the REAL Apply with a real temp-dir agent.json and a call-recording runner; calling seedEscrowStorageID directly cannot see the early return, which IS the defect. Production wiring (pbsdr.NewManager(..., cfg.SourcePath, ...)) is asserted by walking main.go's AST, not by strings.Contains, which a commented-out call also satisfies. Red-proofs, each with the mutation asserted applied: removing the new call makes Scenario A fail on today's tree (it did, with the intended message); removing the early return makes the zero-Proxmox-calls assertion fail (it did). go build / go vet / go test ./... green (29 packages), run separately from this commit.
docs: a comment claimed the hub reads a field it has no field for (R-260)
2026-08-08 16:29:13 +02:00 · 2026-08-08 08:46:17 +02:00
4 changed files with 321 additions and 3 deletions
@@ -1,3 +1,54 @@
+## v0.128.0 — the escrow seed is asserted every tick, not remembered once (2026-08-08, R-221)
+
+**A rebuilt box could not run the escrow ceremony at all, and there was no way forward from inside
+the product.** The preflight refuses on `escrow.pbs_storage_id`; the pbsdr bridge writes that key;
+and it wrote it from exactly one place — `finishConverged`, reached only on the paths that actually
+converge.
+
+**The two things live in different places and die at different times.** The convergence marker is
+host-side (`<agent-state>/pbsdr/marker.json`). The key it seeds is in `agent.json`, which
+`step_agent_config` renders from `base = {}` unless an explicit `--preserve-from` is passed
+(`felhom.eu/scripts/felhom-host-install.sh:2396`, the render at `:2449`, the `O_TRUNC` write at
+`:2579`; the flag at `:1246`, defaulting empty at `:256`) — **and the render never writes an
+`escrow` section at all.** So a rebuild keeps the marker and takes the key: same descriptor, same
+hash, early return, and the seed never runs again into a config that no longer has it.
+
+**Fixed by asserting rather than remembering.** `Apply` now re-asserts the seed *before* the
+idempotent early return. `seedEscrowStorageID` is unchanged and still never clobbers a different
+existing value — an operator's own choice outranks the descriptor's, with a warning naming both.
+
+**The early return is KEPT.** It exists so a converged box does not re-run Proxmox operations every
+60 s, and `TestSeedReasserted_OnConvergedTick_WithZeroProxmoxCalls` asserts **zero** recorded runner
+calls on that tick — so a "fix" that simply deleted the return would fail. Cost of the re-assert: one
+small file read plus a JSON parse per tick, no exec, no network, and an early return once the value
+matches.
+
+**A seed failure can never un-converge the box:** Warn plus a message on the published status,
+exactly as `finishConverged` does it — no marker write, no state change. Pinned by
+`TestSeedReassertFailure_DoesNotUnconverge`.
+
+Tests drive the real `Apply` with a real temp-dir `agent.json` and a call-recording runner; calling
+`seedEscrowStorageID` directly cannot see the early return, which IS the defect. The production
+wiring (`pbsdr.NewManager(..., cfg.SourcePath, ...)`) is asserted by walking `main.go`'s **AST**, not
+by `strings.Contains`, which a commented-out call also satisfies.
+
+Red-proofs: removing the new call makes Scenario A fail on today's tree; removing the early return
+makes the zero-Proxmox-calls assertion fail.
+
+## (no version bump) — a comment that claimed the hub reads a field it has no field for (2026-08-08, R-260)
+
+Comment-only; no behaviour, no wire change, nothing to rebuild.
+
+`HostReport.SelfUpdatePending` / `SelfUpdatePendingVersion` carried the sentence *"The hub reads an
+absent field as pending=false, the correct default."* **The hub has no field for either**, so it reads
+nothing — present or absent — and `encoding/json` discards them on arrival. The sentence described an
+intent rather than the code and read as settled for long enough that a class sweep had to find it.
+
+The emission is correct and stays: the agent reports the truth, and the fault is entirely in the
+receiving. The missing consumer is tracked as **R-264** (OPEN), and
+`felhom.eu/scripts/wire_contract_gate.py` now refuses any NEW field of this shape while recording the
+existing ones as explicit, reasoned allowlist entries rather than silence.
+
 ## v0.127.0 — a mount Felhom itself made is not "something else" (2026-08-06, R-220)

 **After a rebuild the customer's own drives could not be re-attached, and the refusal named an action
@@ -68,8 +68,14 @@ type HostReport struct {
 	// report is stored opaquely hub-side, so these additive fields need no hub-schema change.
 	// Both are `omitempty` (the Wireguard precedent): in the steady state (no update in flight)
 	// they are absent — which keeps the cross-repo host-report golden contract byte-stable without
-	// a hub change. They appear only while an update is pending. The hub reads an absent field as
-	// pending=false, the correct default.
+	// a hub change. They appear only while an update is pending.
+	//
+	// ⚠ CORRECTED 2026-08-08 (R-260). This comment used to end "The hub reads an absent field as
+	// pending=false, the correct default." THE HUB HAS NO FIELD FOR EITHER OF THESE, so it reads
+	// nothing — present or absent — and encoding/json discards them on arrival. The sentence
+	// described an intent, not the code, and it read as settled for long enough that a sweep had to
+	// find it. The emission is correct and stays; the missing consumer is tracked as R-264, and
+	// `felhom.eu/scripts/wire_contract_gate.py` now refuses any NEW field of this shape.
 	SelfUpdatePending        bool   `json:"selfupdate_pending,omitempty"`
 	SelfUpdatePendingVersion string `json:"selfupdate_pending_version,omitempty"`

@@ -283,7 +283,36 @@ func (m *Manager) Apply(ctx context.Context, fetched bool, block *hub.WirePBSDR)
 	h := descriptorHash(block)
 	cf := m.loadConsumedFailed()
 	if mk := m.loadMarker(); mk != nil && mk.Hash == h && (cf == nil || cf.Hash != h) {
-		m.setStatus(&hub.PBSDRStatus{State: mk.State, StorageID: block.StorageID, Namespace: block.Namespace, AppliedAt: mk.AppliedAt})
+		// R-221: RE-ASSERT THE SEED, DO NOT REMEMBER IT. The marker records that this descriptor
+		// converged; it says nothing about whether the file the seed writes still exists.
+		//
+		// The two live in different places and die at different times. The marker is host-side
+		// (`<agent-state>/pbsdr/`, markerPath above); the seed's target is `agent.json`, and the
+		// installer's `step_agent_config` renders that file from `base = {}` unless an explicit
+		// `--preserve-from` is passed — it NEVER writes an `escrow` section — then replaces it with
+		// O_TRUNC (felhom-host-install.sh:2396, :2449, :2579; the flag is :1246, defaulting empty at
+		// :256). So a rebuild leaves the marker and takes the seed, the hash still matches, this
+		// branch returns, and `escrow.pbs_storage_id` is never written again. The customer then
+		// cannot run the escrow ceremony AT ALL: handleEscrowPreflight fails the `pbs_storage_id`
+		// row and the wizard refuses, with no way forward from inside the product.
+		//
+		// A rebuild is only the case that was measured. The same hole opens for a hand-edited or
+		// restored config, which is the honest reason this is a seam fix rather than an installer
+		// fix — the seed must be a thing the loop asserts, not a thing it did once.
+		//
+		// COST: this runs on the converged path, i.e. every tick (60 s) forever. It is one small
+		// file read plus a JSON parse — no exec, no network, no Proxmox call — and seedEscrowStorageID
+		// returns early once the value matches. That is the whole reason it is affordable here.
+		//
+		// IT MUST NEVER UN-CONVERGE THE BOX: a failure is a Warn plus a message on the published
+		// status, exactly as finishConverged does it. No marker write, no state change, no retry
+		// storm — the early return below still happens either way.
+		msg := ""
+		if err := m.seedEscrowStorageID(block.StorageID); err != nil {
+			msg = "escrow.pbs_storage_id seed failed: " + err.Error() + " (set it manually before the ceremony)"
+			m.logger.Warn("pbsdr: " + msg)
+		}
+		m.setStatus(&hub.PBSDRStatus{State: mk.State, StorageID: block.StorageID, Namespace: block.Namespace, AppliedAt: mk.AppliedAt, Message: msg})
 		return // idempotent: this exact descriptor already converged
 	}

@@ -0,0 +1,232 @@
+package pbsdr
+
+import (
+	"context"
+	"encoding/json"
+	"go/ast"
+	"go/parser"
+	"go/printer"
+	"go/token"
+	"io"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"gitea.dooplex.hu/admin/felhom-agent/internal/hub"
+)
+
+// R-221 — the escrow seed must be ASSERTED on every converged tick, not remembered.
+//
+// These tests drive the real Apply() with a real temp-dir agent.json and a call-recording runner.
+// Calling seedEscrowStorageID directly would prove nothing: the defect IS the early return in
+// Apply, and a test that steps around it cannot see it.
+
+// convergedMarker writes a marker whose hash matches the block, i.e. puts the manager on exactly
+// the idempotent path where the seed used to be skipped.
+func convergedMarker(t *testing.T, m *Manager, block *hub.WirePBSDR, state string) {
+	t.Helper()
+	if err := m.writeState(m.markerPath(), marker{
+		Hash: descriptorHash(block), State: state, AppliedAt: "2026-08-08T00:00:00Z",
+	}); err != nil {
+		t.Fatalf("write marker: %v", err)
+	}
+}
+
+func escrowStorageID(t *testing.T, cfgPath string) string {
+	t.Helper()
+	raw, err := os.ReadFile(cfgPath)
+	if err != nil {
+		t.Fatalf("read config: %v", err)
+	}
+	var doc struct {
+		Escrow struct {
+			PBSStorageID string `json:"pbs_storage_id"`
+		} `json:"escrow"`
+	}
+	if err := json.Unmarshal(raw, &doc); err != nil {
+		t.Fatalf("parse config: %v", err)
+	}
+	return doc.Escrow.PBSStorageID
+}
+
+// drBlock mirrors the existing valid fixture (manager_test.go descriptor()) so that validate()
+// passes and Apply actually reaches the marker check — the branch these tests are about.
+func drBlock(storageID string) *hub.WirePBSDR {
+	return &hub.WirePBSDR{
+		Enabled: true, StorageID: storageID, PBSTunnelIP: "10.77.0.1",
+		Datastore: "felhom-offsite", Namespace: "peti", TokenID: "felhom@pbs!peti",
+		Fingerprint: testFP,
+	}
+}
+
+// SCENARIO A — the seed is re-asserted on a converged box, and nothing else happens.
+//
+// THIS IS THE TEST THAT MATTERS. It must fail against the pre-R-221 tree; if it passes there, it is
+// not testing the defect and that is the finding.
+func TestSeedReasserted_OnConvergedTick_WithZeroProxmoxCalls(t *testing.T) {
+	r := &fakeRunner{}
+	st := &fakeStorage{found: true, active: []bool{true}}
+	c := &fakeConsumer{}
+	m, cfgPath := newTestManager(t, r, st, c)
+
+	block := drBlock("felhom-pbs-dr")
+	convergedMarker(t, m, block, "applied")
+
+	// the shape a rebuild leaves behind: escrow section present, pbs_storage_id GONE
+	if got := escrowStorageID(t, cfgPath); got != "" {
+		t.Fatalf("precondition: config already carries a storage id %q", got)
+	}
+
+	m.Apply(context.Background(), true, block)
+
+	if got := escrowStorageID(t, cfgPath); got != "felhom-pbs-dr" {
+		t.Errorf("the converged tick did not re-assert the seed: escrow.pbs_storage_id = %q, want %q.\n"+
+			"This is R-221: the marker survives a rebuild, the descriptor hash still matches, the early "+
+			"return fires and the seed never runs into the config that no longer has it — so the customer "+
+			"cannot run the escrow ceremony at all.", got, "felhom-pbs-dr")
+	}
+
+	// ...and the idempotent path is STILL idempotent. This assertion is not decorative: without it
+	// a "fix" that simply deletes the early return would pass the line above.
+	if calls := r.recorded(); len(calls) != 0 {
+		t.Errorf("a converged tick must execute ZERO Proxmox commands; got %d: %+v", len(calls), calls)
+	}
+}
+
+// SCENARIO B — an operator's own different value survives, and the warning names both.
+func TestSeedReasserted_NeverClobbersAnOperatorValue(t *testing.T) {
+	r := &fakeRunner{}
+	m, cfgPath := newTestManager(t, r, &fakeStorage{found: true, active: []bool{true}}, &fakeConsumer{})
+
+	if err := os.WriteFile(cfgPath, []byte(
+		`{"log_level":"info","escrow":{"posture":"zero_knowledge","pbs_storage_id":"operator-chosen"},`+
+			`"custom_unknown":{"keep":1}}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	block := drBlock("hub-chosen")
+	convergedMarker(t, m, block, "applied")
+
+	m.Apply(context.Background(), true, block)
+
+	if got := escrowStorageID(t, cfgPath); got != "operator-chosen" {
+		t.Errorf("a value a person put there was overwritten by the descriptor: got %q, want %q", got, "operator-chosen")
+	}
+	// unknown keys must still round-trip
+	raw, _ := os.ReadFile(cfgPath)
+	if !strings.Contains(string(raw), "custom_unknown") {
+		t.Error("an unknown config key was dropped by the re-assert")
+	}
+}
+
+// SCENARIO C — the ceremony preflight's live read sees the re-asserted value with NO restart.
+//
+// The preflight itself lives in internal/localapi and reads the file through config.Load; what this
+// asserts is the half that belongs to this package: after a converged tick, THE FILE ON DISK carries
+// the id, so any live re-read is green. The daemon is never restarted in this test because it is
+// never started — which is the point.
+func TestSeedReasserted_IsVisibleOnDiskImmediately(t *testing.T) {
+	r := &fakeRunner{}
+	m, cfgPath := newTestManager(t, r, &fakeStorage{found: true, active: []bool{true}}, &fakeConsumer{})
+	block := drBlock("felhom-pbs-dr")
+	convergedMarker(t, m, block, "applied")
+
+	// preflight's predicate BEFORE: storageID == "" → the row is NOT OK
+	if escrowStorageID(t, cfgPath) != "" {
+		t.Fatal("precondition")
+	}
+	m.Apply(context.Background(), true, block)
+	// preflight's predicate AFTER, from the same file the ceremony subprocess loads
+	if id := escrowStorageID(t, cfgPath); id == "" {
+		t.Error("the preflight row would still be NOT OK after a converged tick")
+	}
+}
+
+// A seed failure must NEVER un-converge the box: no marker rewrite, no state change, and the status
+// still reports the marker's converged state — with the failure surfaced as a message.
+func TestSeedReassertFailure_DoesNotUnconverge(t *testing.T) {
+	r := &fakeRunner{}
+	m, cfgPath := newTestManager(t, r, &fakeStorage{found: true, active: []bool{true}}, &fakeConsumer{})
+	block := drBlock("felhom-pbs-dr")
+	convergedMarker(t, m, block, "applied")
+	markerBefore, err := os.ReadFile(m.markerPath())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// make the seed fail in a way it cannot recover from: unparseable config
+	if err := os.WriteFile(cfgPath, []byte(`{ this is not json`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	m.Apply(context.Background(), true, block)
+
+	after, err := os.ReadFile(m.markerPath())
+	if err != nil {
+		t.Fatalf("the marker was removed by a seed failure: %v", err)
+	}
+	if string(after) != string(markerBefore) {
+		t.Error("a seed failure rewrote the convergence marker — it must not touch state")
+	}
+	if calls := r.recorded(); len(calls) != 0 {
+		t.Errorf("a seed failure must not trigger Proxmox work; got %+v", calls)
+	}
+	st := m.Status()
+	if st == nil || st.State != "applied" {
+		t.Errorf("a seed failure must leave the box converged; status = %+v", st)
+	}
+	if st != nil && !strings.Contains(st.Message, "seed failed") {
+		t.Errorf("a seed failure must be surfaced on the status, got message %q", st.Message)
+	}
+}
+
+// SEAM WIRING — production must construct the manager with the live config path, or the whole seed
+// leg is inert. Three shipped defects in this project were fully green while their seam was never
+// wired, so this walks main.go's AST for the actual call rather than grepping: a commented-out call
+// satisfies strings.Contains, and an AST walk cannot see a comment.
+func TestProductionWiring_NewManagerGetsTheLiveConfigPath(t *testing.T) {
+	path := filepath.Join("..", "..", "cmd", "felhom-agent", "main.go")
+	fset := token.NewFileSet()
+	f, err := parser.ParseFile(fset, path, nil, 0) // comments not even collected
+	if err != nil {
+		t.Fatalf("parse main.go: %v", err)
+	}
+	found := false
+	ast.Inspect(f, func(n ast.Node) bool {
+		call, ok := n.(*ast.CallExpr)
+		if !ok {
+			return true
+		}
+		sel, ok := call.Fun.(*ast.SelectorExpr)
+		if !ok || sel.Sel.Name != "NewManager" {
+			return true
+		}
+		if pkg, ok := sel.X.(*ast.Ident); !ok || pkg.Name != "pbsdr" {
+			return true
+		}
+		// signature: (runner, px, hubc, stateDir, secretDir, configPath, logger)
+		if len(call.Args) < 6 {
+			t.Errorf("pbsdr.NewManager called with %d args, expected >= 6", len(call.Args))
+			return false
+		}
+		var buf strings.Builder
+		if err := printNode(&buf, fset, call.Args[5]); err != nil {
+			t.Fatalf("print arg: %v", err)
+		}
+		got := buf.String()
+		if !strings.Contains(got, "SourcePath") {
+			t.Errorf("pbsdr.NewManager's configPath argument is %q, which is not the live config path.\n"+
+				"With an empty or wrong path seedEscrowStorageID returns nil immediately and the entire "+
+				"R-221 fix is inert while every test above still passes.", got)
+		}
+		found = true
+		return false
+	})
+	if !found {
+		t.Error("no pbsdr.NewManager call found in main.go — the manager is not constructed in production")
+	}
+}
+
+func printNode(w io.Writer, fset *token.FileSet, n ast.Node) error {
+	return printer.Fprint(w, fset, n)
+}