feat(reconcile): re-assert pool membership after restore-over-existing (campaign-2 R2, v0.74.0)

Pool membership is what lets the pool-scoped token reach a guest; pct restore
--pool sets it only at CREATE, so a restore over an existing VMID drops the guest
from the felhom pool and 403s the next restore-test/DR on VM.Audit. This empty-pool
state is the true root cause of the campaign's "R1" (bind-mount restore failing was
a symptom — restore-test's existing bind neutralization never ran without config-read).

Add Client.PoolAddVMID (PUT /pools, additive+idempotent, Pool.Allocate) and call it
in bring-up after liveness when spec.Pool!="" — warn-not-fail on a hiccup (liveness
wins). B3 scratch-teardown 403 diagnosed as a cascade (restoretest already passes
Pool). Role/ACL untouched. Tests + red-proof.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6

CHANGELOG.md

@@ -1,3 +1,29 @@
+## v0.74.0 — pool membership re-asserted after restore-over-existing (campaign-2 R2) (2026-07-07)
+
+Closes campaign-2 finding **R2** (`felhom.eu/documentation/tests/CAMPAIGN-2-2026-07-07.md`). Pool
+membership is what lets the pool-scoped `FelhomAgentGuest` token reach a guest (the grant applies only
+to pool MEMBERS). `pct restore --pool` sets membership at CREATE, but a restore **over an existing
+VMID** (the host-loss/finale path) never re-applies it — and no code re-added a guest to the pool — so
+every destroy-restore silently dropped membership, and the NEXT `--selftest=restore-test`/DR 403'd on
+`VM.Audit`/`VM.Allocate`. That empty-pool state is the true root cause of the campaign's "R1" (the
+bind-mount restore failure was a symptom: with no config-read, restore-test's *existing, correct*
+bind-mount neutralization never ran).
+
+- **`Client.PoolAddVMID(ctx, pool, vmid)`** (`internal/proxmox/mutate.go`): `PUT /pools/{pool}`
+  `vms={vmid}` — PVE-additive (merge, not replace), idempotent (already-member swallowed), needs
+  `Pool.Allocate` (the token has it). Sync — no UPID.
+- **bring-up re-asserts** (`internal/reconcile/bringup.go`): after liveness is proven, if `spec.Pool
+  != ""`, call `PoolAddVMID`. A pool-add hiccup is surfaced as a LOUD warning + `res.StartWarnings`
+  but must NOT flip a healthy running guest's verdict (membership matters for the NEXT op).
+- **B3 (scratch teardown 403) — diagnosed, no code:** `restoretest.go` already passes
+  `Pool: DefaultPool` for scratch restores, so the campaign's `VM.Allocate` teardown 403 was a
+  CASCADE of the bind-mount restore failing (half-built guest outside any pool), not an independent
+  gap. A re-run on the healed pool confirms.
+- Tests: `PoolAddVMID` PUT shape + idempotency + real-error-surfaces + validation (`pool_test.go`);
+  bring-up re-asserts when `Pool!=""` (red-proof: pre-fix no-call → FAIL, demonstrated), no-pool→no-call,
+  pool-add failure warns-but-passes (liveness wins, guest kept). Role/ACL untouched — the fault was
+  membership, not privileges.
+
 ## v0.73.0 — F2 mount-role fallback: enrolled user-data drives are ejectable/decommissionable again (2026-07-06)
 
 Closes campaign finding **F2** (`felhom.eu/documentation/audits/CAMPAIGN-nomercy-2026-07-06.md` +

REUSE.md

@@ -79,6 +79,7 @@
 | `Client.WaitTask` | internal/proxmox/task.go | `WaitTask(ctx, upid, opts) (TaskStatus, error)` | asserting EVERY mutating op | POST 200 ≠ success; authz can fail at task exec; `AllowWarnings` opt-in |
 | `Client.Pool` | internal/proxmox/query.go | `Pool(ctx, name) (PoolInfo, error)` | felhom-pool membership (the ownership registry, A1) | Needs `Pool.Audit` at `/pool/<name>` (host-install v1.9.0+); `Pool.Allocate` does NOT satisfy the read; members can be storages (type `storage`, vmid 0) — filter them |
 | `Client` mutate wrappers (`RestoreLXC/Vzdump/DestroyLXC/Snapshot/Rollback/SetConfig/ResizeLXC/Start/Stop`) | internal/proxmox/mutate.go | return `(upid, error)` | all API mutations | Async → always pair with WaitTask; route via gate/queue, not ad-hoc |
+| `Client.PoolAddVMID` | internal/proxmox/mutate.go | `PoolAddVMID(ctx, pool, vmid) error` | re-assert pool membership after a restore-over-existing (campaign-2 R2) | SYNC (no UPID, don't WaitTask); PVE `PUT /pools` is additive (merge, not replace) — `delete=1` removes; idempotent (already-member swallowed); needs `Pool.Allocate` at `/pool/<pool>`. `pct restore --pool` sets membership only at CREATE — a restore over an existing vmid drops it, so bring-up re-asserts post-restore |
 | `TLSConfig.build` / `normalizeFingerprint` | internal/proxmox/tls.go | `build() (*tls.Config, error)` | PVE leaf-cert SHA-256 pinning | No insecure default |
 | `pinnedTLS` | internal/pbs/pin.go | `pinnedTLS(fingerprint) (*tls.Config, error)` | PBS leaf pinning | Same model as PVE; 64-hex fingerprint normalized |
 | `hub.Client.Report` | internal/hub/client.go | `Report(ctx, *HostReport) (*ControlEnvelope, error)` | the heartbeat | Typed `TransportError`/`HTTPError`, never contain the bearer token |

internal/proxmox/mutate.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"net/url"
 	"strconv"
+	"strings"
 )
 
 // Async mutating operations. Each is API-token-covered (the FelhomAgent role) and
@@ -185,6 +186,36 @@ func (c *Client) SetConfig(ctx context.Context, vmid int, params map[string]stri
 	return c.dataString(ctx, http.MethodPut, path, v)
 }
 
+// PoolAddVMID adds a guest to a PVE pool via PUT /pools/{pool} with vms={vmid}.
+//
+// Membership is what makes a pool-scoped token (FelhomAgentGuest @ /pool/<pool>) reach a guest: the
+// grant applies only to pool MEMBERS. `pct restore --pool` sets membership at CREATE, but a restore
+// OVER AN EXISTING VMID (the P9/host-loss finale) does NOT re-apply it — so a destroy-restore
+// silently drops the guest from the pool and 403s the NEXT restore-test/DR (campaign-2 R2). This
+// re-asserts it after such a restore.
+//
+// PVE semantics: `PUT /pools/{poolid}` with `vms` is ADDITIVE (a merge) — `delete=1` is required to
+// REMOVE, so passing a single vmid adds it without disturbing existing members. Adding a guest that
+// is already a member is treated as a no-op success (idempotent): PVE reports "already" in the error
+// body, which we swallow. Requires Pool.Allocate at /pool/<pool> (the token has it).
+func (c *Client) PoolAddVMID(ctx context.Context, pool string, vmid int) error {
+	if pool == "" || vmid == 0 {
+		return fmt.Errorf("proxmox: PoolAddVMID needs pool and vmid")
+	}
+	v := url.Values{}
+	v.Set("vms", strconv.Itoa(vmid))
+	path := "/pools/" + url.PathEscape(pool)
+	_, err := c.dataString(ctx, http.MethodPut, path, v)
+	if err != nil {
+		// Idempotent: a guest already in the pool is success, not a failure.
+		if ae, ok := err.(*APIError); ok && strings.Contains(strings.ToLower(ae.Body), "already") {
+			return nil
+		}
+		return err
+	}
+	return nil
+}
+
 // ResizeLXC grows a guest volume via PUT /nodes/{node}/lxc/{vmid}/resize
 // (token-covered: VM.Config.Disk + Datastore.AllocateSpace). Returns the UPID.
 //

internal/proxmox/pool_test.go

@@ -0,0 +1,96 @@
+package proxmox
+
+import (
+	"context"
+	"io"
+	"net/http"
+	"testing"
+)
+
+// PoolAddVMID issues PUT /pools/{pool} with vms={vmid} and treats an already-member error as success.
+func TestPoolAddVMID_PUTShape(t *testing.T) {
+	var gotMethod, gotPath, gotVMS string
+	d := &mockDoer{fn: func(r *http.Request) (*http.Response, error) {
+		gotMethod = r.Method
+		gotPath = r.URL.Path
+		body, _ := io.ReadAll(r.Body) // the client encodes url.Values into the request body
+		for _, kv := range splitAmp(string(body)) {
+			if k, v, ok := cut(kv, "="); ok && k == "vms" {
+				gotVMS = v
+			}
+		}
+		return jsonResp(http.StatusOK, `{"data":null}`), nil
+	}}
+	c := newTestClient(d)
+	if err := c.PoolAddVMID(context.Background(), "felhom", 9201); err != nil {
+		t.Fatalf("PoolAddVMID: %v", err)
+	}
+	if gotMethod != http.MethodPut {
+		t.Errorf("method = %q, want PUT", gotMethod)
+	}
+	if gotPath != "/api2/json/pools/felhom" {
+		t.Errorf("path = %q, want /api2/json/pools/felhom", gotPath)
+	}
+	if gotVMS != "9201" {
+		t.Errorf("vms param = %q, want 9201", gotVMS)
+	}
+}
+
+// Idempotent: an "already in pool" error from PVE is swallowed as success.
+func TestPoolAddVMID_IdempotentOnAlreadyMember(t *testing.T) {
+	d := &mockDoer{fn: func(_ *http.Request) (*http.Response, error) {
+		return jsonResp(http.StatusInternalServerError, `{"data":null,"errors":{"vms":"VM 9201 is already in pool 'felhom'"}}`), nil
+	}}
+	c := newTestClient(d)
+	if err := c.PoolAddVMID(context.Background(), "felhom", 9201); err != nil {
+		t.Fatalf("already-member should be idempotent success, got: %v", err)
+	}
+}
+
+// A real failure (e.g. a 403 with no "already") is surfaced, not swallowed.
+func TestPoolAddVMID_RealErrorSurfaces(t *testing.T) {
+	d := &mockDoer{fn: func(_ *http.Request) (*http.Response, error) {
+		return jsonResp(http.StatusForbidden, `Permission check failed (/pool/felhom, Pool.Allocate)`), nil
+	}}
+	c := newTestClient(d)
+	if err := c.PoolAddVMID(context.Background(), "felhom", 9201); err == nil {
+		t.Fatal("expected a real 403 to surface, got nil")
+	}
+}
+
+func TestPoolAddVMID_Validation(t *testing.T) {
+	c := newTestClient(&mockDoer{fn: func(_ *http.Request) (*http.Response, error) {
+		t.Fatal("should not issue a request on bad input")
+		return nil, nil
+	}})
+	if err := c.PoolAddVMID(context.Background(), "", 9201); err == nil {
+		t.Error("empty pool should error")
+	}
+	if err := c.PoolAddVMID(context.Background(), "felhom", 0); err == nil {
+		t.Error("zero vmid should error")
+	}
+}
+
+// tiny helpers (avoid pulling strings.Split into an assertion path that could mask a bug)
+func splitAmp(s string) []string {
+	var out []string
+	cur := ""
+	for _, r := range s {
+		if r == '&' {
+			out = append(out, cur)
+			cur = ""
+			continue
+		}
+		cur += string(r)
+	}
+	return append(out, cur)
+}
+
+func cut(s, sep string) (string, string, bool) {
+	for i := 0; i+len(sep) <= len(s); i++ {
+		if s[i:i+len(sep)] == sep {
+			return s[:i], s[i+len(sep):], true
+		}
+	}
+	return s, "", false
+}

internal/reconcile/bringup.go

@@ -344,6 +344,22 @@ func (e *Engine) runBringUp(ctx context.Context, spec BringUpSpec, res *BringUpR
 		return
 	}
 
+	// 6b. Re-assert pool membership (campaign-2 R2). `pct restore --pool` sets membership only at
+	//     CREATE; a restore OVER AN EXISTING VMID (host-loss finale) does not re-apply it, silently
+	//     dropping the guest from the pool and 403-ing the NEXT restore-test/DR. Idempotent on a
+	//     fresh-VMID restore that already got membership. This runs AFTER liveness is proven: a
+	//     pool-add hiccup is surfaced LOUD as a warning but must NOT flip a healthy, running guest's
+	//     verdict to fail (membership matters for the next op, not this guest's boot).
+	if spec.Pool != "" {
+		if err := e.api.PoolAddVMID(ctx, spec.Pool, spec.VMID); err != nil {
+			e.logger.Error("bring-up: pool membership re-assert FAILED (next restore-test/DR may 403); guest is healthy",
+				"vmid", spec.VMID, "pool", spec.Pool, "err", err)
+			res.StartWarnings = append(res.StartWarnings, fmt.Sprintf("pool re-assert failed (pool=%s): %v", spec.Pool, err))
+		} else {
+			e.logger.Info("bring-up: pool membership re-asserted", "vmid", spec.VMID, "pool", spec.Pool)
+		}
+	}
+
 	// 7. Success — KEEP the guest; mark the owning entry terminal so Recover ignores it.
 	res.Pass = true
 	res.Verified = "boot+running"

internal/reconcile/bringup_test.go

@@ -629,3 +629,91 @@ func lockBackoffFast(t *testing.T) {
 	configLockBackoff = time.Millisecond
 	t.Cleanup(func() { configLockBackoff = prev })
 }
+
+// --- Part B (campaign-2 R2): pool re-assertion after bring-up ------------------------------------
+
+// poolAddsFor returns the recorded PoolAddVMID calls for a vmid.
+func poolAddsFor(api *fakeAPI, vmid int) []poolAddCall {
+	var out []poolAddCall
+	for _, c := range api.poolAdds {
+		if c.vmid == vmid {
+			out = append(out, c)
+		}
+	}
+	return out
+}
+
+// A bring-up with a Pool set MUST re-assert pool membership after the restore (so a
+// restore-over-existing that dropped membership is healed for the NEXT restore-test/DR).
+// COMPANION RED-PROOF: on the pre-fix code (no PoolAddVMID call) poolAddsFor is empty → this FAILS.
+func TestRunBringUp_ReassertsPoolMembership(t *testing.T) {
+	const vmid = 8100
+	api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}}
+	e, _, q := newEngine(t, api, EmptyProvider{})
+	defer q.Close()
+
+	res := e.RunBringUp(context.Background(), BringUpSpec{
+		Mode: ModeDRGuestLoss, Archive: "local:backup/cust.tar.zst", VMID: vmid,
+		RestoreStorage: "local-lvm", Pool: "felhom",
+	})
+	if res.Err != nil || !res.Pass {
+		t.Fatalf("bring-up must pass, got %+v", res)
+	}
+	adds := poolAddsFor(api, vmid)
+	if len(adds) != 1 || adds[0].pool != "felhom" {
+		t.Fatalf("pool membership not re-asserted: poolAdds=%+v", api.poolAdds)
+	}
+}
+
+// No Pool → no PoolAddVMID call (a broad-token restore needs no pool).
+func TestRunBringUp_NoPool_NoPoolAdd(t *testing.T) {
+	const vmid = 8101
+	api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}}
+	e, _, q := newEngine(t, api, EmptyProvider{})
+	defer q.Close()
+
+	res := e.RunBringUp(context.Background(), BringUpSpec{
+		Mode: ModeProvision, Archive: "local:backup/golden.tar.zst", VMID: vmid,
+		RestoreStorage: "local-lvm", Hostname: "h",
+	})
+	if res.Err != nil || !res.Pass {
+		t.Fatalf("bring-up must pass, got %+v", res)
+	}
+	if len(api.poolAdds) != 0 {
+		t.Fatalf("no Pool set → PoolAddVMID must not be called: %+v", api.poolAdds)
+	}
+}
+
+// A pool-add FAILURE must NOT flip a healthy, running guest's verdict — it surfaces as a LOUD warning
+// (membership matters for the NEXT op, not this guest's boot). Liveness wins.
+func TestRunBringUp_PoolAddFailure_WarnsButPasses(t *testing.T) {
+	const vmid = 8102
+	api := &fakeAPI{
+		cfg:        map[int]proxmox.GuestConfig{vmid: scratchCfg()},
+		poolAddErr: errors.New("proxmox: PUT /pools/felhom -> HTTP 500: transient"),
+	}
+	e, _, q := newEngine(t, api, EmptyProvider{})
+	defer q.Close()
+
+	res := e.RunBringUp(context.Background(), BringUpSpec{
+		Mode: ModeDRGuestLoss, Archive: "local:backup/cust.tar.zst", VMID: vmid,
+		RestoreStorage: "local-lvm", Pool: "felhom",
+	})
+	if res.Err != nil || !res.Pass {
+		t.Fatalf("a pool-add hiccup must NOT fail a healthy guest, got %+v", res)
+	}
+	// the guest is KEPT (not torn down) despite the pool-add failure.
+	if len(api.destroys) != 0 {
+		t.Fatalf("pool-add failure must not trigger teardown: %+v", api.destroys)
+	}
+	// the failure is surfaced as a warning.
+	found := false
+	for _, w := range res.StartWarnings {
+		if strings.Contains(w, "pool re-assert failed") {
+			found = true
+		}
+	}
+	if !found {
+		t.Fatalf("pool-add failure must surface as a warning: %+v", res.StartWarnings)
+	}
+}

internal/reconcile/engine_test.go

@@ -49,6 +49,23 @@ type fakeAPI struct {
 	waits    []string
 	waitOpts []proxmox.WaitOptions // parallel to waits: the options each WaitTask was called with
 	listErr  error
+
+	// poolAdds records (pool, vmid) for each PoolAddVMID; poolAddErr backs the failure path.
+	poolAdds   []poolAddCall
+	poolAddErr error
+}
+
+type poolAddCall struct {
+	pool string
+	vmid int
+}
+
+func (f *fakeAPI) PoolAddVMID(_ context.Context, pool string, vmid int) error {
+	f.mu.Lock()
+	f.poolAdds = append(f.poolAdds, poolAddCall{pool: pool, vmid: vmid})
+	err := f.poolAddErr
+	f.mu.Unlock()
+	return err
 }
 
 type resizeCall struct {

internal/reconcile/state.go

@@ -172,6 +172,9 @@ type GuestAPI interface {
 	ResizeLXC(ctx context.Context, vmid int, disk, size string) (string, error)
 	// RestoreLXC restores an archive into a (fresh) vmid — the create path (slice 6). Async → UPID.
 	RestoreLXC(ctx context.Context, opts proxmox.RestoreLXCOptions) (string, error)
+	// PoolAddVMID re-asserts pool membership after a restore-over-existing (campaign-2 R2). Sync (no
+	// UPID); idempotent. Membership is what lets the pool-scoped token reach the guest next time.
+	PoolAddVMID(ctx context.Context, pool string, vmid int) error
 	// DestroyLXC destroys a guest — the scratch-teardown primitive (slice 6). Async → UPID.
 	// Destructive-class; the engine only ever issues it for an agent-tagged scratch guest
 	// (benign by provenance) via the gate.