fix(pools): atomic create-with-target and schema-valid set-auth-reset response

Author: nnemirovsky

cmd/sluice/cred_test.go

@@ -2573,7 +2573,7 @@ func TestFinding3Round9_StoreGatedVaultDeleteOnLivePoolMember(t *testing.T) {
 	if err := db.AddCredentialMeta("pool_mem", "oauth", "https://auth.example.com/token"); err != nil {
 		t.Fatalf("AddCredentialMeta: %v", err)
 	}
-	if err := db.CreatePoolWithMembers("codex_pool", "failover", []string{"pool_mem"}); err != nil {
+	if err := db.CreatePoolWithMembers("codex_pool", "failover", []string{"pool_mem"}, ""); err != nil {
 		t.Fatalf("CreatePoolWithMembers: %v", err)
 	}
 	_ = db.Close()
@@ -2692,7 +2692,7 @@ func TestFinding3Round9_TOCTOUInterleaveStoreGatesVaultDelete(t *testing.T) {
 			if e != nil {
 				return
 			}
-			_ = pdb.CreatePoolWithMembers("codex_pool", "failover", []string{"racer"})
+			_ = pdb.CreatePoolWithMembers("codex_pool", "failover", []string{"racer"}, "")
 			_ = pdb.Close()
 		}()
 

cmd/sluice/main_test.go

@@ -1778,7 +1778,7 @@ func seedRecoveryPool(t *testing.T, db *store.Store, members []string) {
 			t.Fatalf("add credential meta %q: %v", m, err)
 		}
 	}
-	if err := db.CreatePoolWithMembers(pool, "", members); err != nil {
+	if err := db.CreatePoolWithMembers(pool, "", members, ""); err != nil {
 		t.Fatalf("create pool %q: %v", pool, err)
 	}
 }

cmd/sluice/pool_test.go

@@ -502,7 +502,7 @@ func TestPoolRotateGuardedAgainstConcurrentRemoval(t *testing.T) {
 		_ = db.Close()
 		t.Fatalf("RemovePool: %v", rerr)
 	}
-	if cerr := db.CreatePoolWithMembers("codex", "failover", []string{"acct_a"}); cerr != nil {
+	if cerr := db.CreatePoolWithMembers("codex", "failover", []string{"acct_a"}, ""); cerr != nil {
 		_ = db.Close()
 		t.Fatalf("recreate pool: %v", cerr)
 	}
@@ -534,7 +534,7 @@ func TestPoolRotateGuardedAgainstConcurrentRemoval(t *testing.T) {
 	if _, rerr := db.RemovePool("codex"); rerr != nil {
 		t.Fatalf("final RemovePool: %v", rerr)
 	}
-	if cerr := db.CreatePoolWithMembers("codex", "failover", []string{"acct_a", "acct_b"}); cerr != nil {
+	if cerr := db.CreatePoolWithMembers("codex", "failover", []string{"acct_a", "acct_b"}, ""); cerr != nil {
 		t.Fatalf("final recreate pool: %v", cerr)
 	}
 	rows, lerr := db.ListCredentialHealth()
@@ -607,7 +607,7 @@ func TestPoolRotateEpochScopedRejectsCrossPoolReAdd(t *testing.T) {
 		_ = db.Close()
 		t.Fatalf("RemovePool(P): %v", rerr)
 	}
-	if cerr := db.CreatePoolWithMembers("Q", "failover", []string{"c", "d"}); cerr != nil {
+	if cerr := db.CreatePoolWithMembers("Q", "failover", []string{"c", "d"}, ""); cerr != nil {
 		_ = db.Close()
 		t.Fatalf("recreate c,d into Q: %v", cerr)
 	}

internal/api/server.go

@@ -1856,9 +1856,10 @@ func (s *Server) PostApiPools(w http.ResponseWriter, r *http.Request) { //nolint
 		effectiveStrategy = store.PoolStrategyFailover
 	}
 	out := storePoolToAPI(store.Pool{
-		Name:     req.Name,
-		Strategy: effectiveStrategy,
-		Members:  membersToStorePoolMembers(req.Members),
+		Name:            req.Name,
+		Strategy:        effectiveStrategy,
+		Members:         membersToStorePoolMembers(req.Members),
+		AuthResetTarget: authResetTarget,
 	})
 	if p, err := s.store.GetPool(req.Name); err == nil && p != nil {
 		out = storePoolToAPI(*p)
@@ -1922,7 +1923,11 @@ func (s *Server) PostApiPoolsNameRotate(w http.ResponseWriter, _ *http.Request,
 // routing through poolops.SetAuthResetTarget so the three surfaces cannot
 // drift (channel feature-parity principle). A NUL/newline in the target is a
 // 400 (poolops.ErrInvalidAuthResetTarget); an unknown pool is 404. On success
-// the updated pool is returned so the caller sees the persisted value.
+// the updated pool is returned (200). If the post-write read-back fails the
+// set still succeeded, so 204 No Content is returned rather than a partial
+// Pool object: the OpenAPI Pool schema requires name+strategy+members, and the
+// request body alone cannot reconstruct strategy/members, so echoing it would
+// emit a schema-invalid response.
 func (s *Server) PostApiPoolsNameAuthResetTarget(w http.ResponseWriter, r *http.Request, name string) { //nolint:revive // generated interface name
 	var req SetPoolAuthResetTargetRequest
 	if err := json.NewDecoder(limitedBody(w, r)).Decode(&req); err != nil {
@@ -1937,14 +1942,15 @@ func (s *Server) PostApiPoolsNameAuthResetTarget(w http.ResponseWriter, r *http.
 		writeError(w, status, err.Error(), "")
 		return
 	}
-	w.Header().Set("Content-Type", "application/json")
 	if p, err := s.store.GetPool(name); err == nil && p != nil {
+		w.Header().Set("Content-Type", "application/json")
 		_ = json.NewEncoder(w).Encode(storePoolToAPI(*p))
 		return
 	}
-	// The set succeeded; a read-back failure must not report failure. Echo
-	// the persisted value from the request instead.
-	_ = json.NewEncoder(w).Encode(Pool{Name: name, AuthResetTarget: &req.AuthResetTarget})
+	// The set succeeded; only the post-write read-back failed. Returning a
+	// partial Pool (name + target, missing the required strategy/members)
+	// would violate the OpenAPI schema, so report success with no body.
+	w.WriteHeader(http.StatusNoContent)
 }
 
 // DeleteApiPoolsName removes a pool. It refuses (409) while any binding still

internal/api/server_test.go

@@ -1561,7 +1561,7 @@ func TestDeleteApiCredentials_PoolGuardVsStoreFault(t *testing.T) {
 				t.Fatalf("seed oauth cred %q: %v", n, err)
 			}
 		}
-		if err := st.CreatePoolWithMembers("p", "failover", []string{"m", "n"}); err != nil {
+		if err := st.CreatePoolWithMembers("p", "failover", []string{"m", "n"}, ""); err != nil {
 			t.Fatalf("create pool: %v", err)
 		}
 
@@ -2801,7 +2801,7 @@ func TestPostApiPools_DuplicateName(t *testing.T) {
 	st := newTestStore(t)
 	enableHTTPChannel(t, st)
 	seedOAuthCred(t, st, "credA", "credB")
-	if err := st.CreatePoolWithMembers("dup_pool", store.PoolStrategyFailover, []string{"credA"}); err != nil {
+	if err := st.CreatePoolWithMembers("dup_pool", store.PoolStrategyFailover, []string{"credA"}, ""); err != nil {
 		t.Fatalf("seed pool: %v", err)
 	}
 	srv := api.NewServer(st, nil, nil, "")
@@ -2851,7 +2851,7 @@ func TestPostApiPools_MemberAlreadyPooled(t *testing.T) {
 	st := newTestStore(t)
 	enableHTTPChannel(t, st)
 	seedOAuthCred(t, st, "credA", "credB")
-	if err := st.CreatePoolWithMembers("pool_one", store.PoolStrategyFailover, []string{"credA"}); err != nil {
+	if err := st.CreatePoolWithMembers("pool_one", store.PoolStrategyFailover, []string{"credA"}, ""); err != nil {
 		t.Fatalf("seed pool: %v", err)
 	}
 	srv := api.NewServer(st, nil, nil, "")
@@ -2955,7 +2955,7 @@ func TestGetApiPoolsName_Status(t *testing.T) {
 	st := newTestStore(t)
 	enableHTTPChannel(t, st)
 	seedOAuthCred(t, st, "credA", "credB")
-	if err := st.CreatePoolWithMembers("pool1", store.PoolStrategyFailover, []string{"credA", "credB"}); err != nil {
+	if err := st.CreatePoolWithMembers("pool1", store.PoolStrategyFailover, []string{"credA", "credB"}, ""); err != nil {
 		t.Fatalf("create pool: %v", err)
 	}
 	srv := api.NewServer(st, nil, nil, "")
@@ -3011,7 +3011,7 @@ func TestPostApiPoolsNameRotate_Success(t *testing.T) {
 	st := newTestStore(t)
 	enableHTTPChannel(t, st)
 	seedOAuthCred(t, st, "credA", "credB")
-	if err := st.CreatePoolWithMembers("pool1", store.PoolStrategyFailover, []string{"credA", "credB"}); err != nil {
+	if err := st.CreatePoolWithMembers("pool1", store.PoolStrategyFailover, []string{"credA", "credB"}, ""); err != nil {
 		t.Fatalf("create pool: %v", err)
 	}
 	srv := api.NewServer(st, nil, nil, "")
@@ -3051,7 +3051,7 @@ func TestPostApiPoolsNameAuthResetTarget(t *testing.T) {
 	st := newTestStore(t)
 	enableHTTPChannel(t, st)
 	seedOAuthCred(t, st, "credA", "credB")
-	if err := st.CreatePoolWithMembers("pool1", store.PoolStrategyFailover, []string{"credA", "credB"}); err != nil {
+	if err := st.CreatePoolWithMembers("pool1", store.PoolStrategyFailover, []string{"credA", "credB"}, ""); err != nil {
 		t.Fatalf("create pool: %v", err)
 	}
 	srv := api.NewServer(st, nil, nil, "")
@@ -3067,7 +3067,12 @@ func TestPostApiPoolsNameAuthResetTarget(t *testing.T) {
 		return rec
 	}
 
-	// Set a target.
+	// Set a target. The 200 path returns the full, schema-complete pool via a
+	// read-back: name+strategy+members are all required by the OpenAPI Pool
+	// schema. (The read-back-failure fallback returns 204 No Content rather
+	// than a partial Pool that would violate that schema — Copilot #2 — but
+	// that path is not reachable here without failing the store mid-handler,
+	// which the concrete *store.Store gives no seam for.)
 	rec := post(`{"auth_reset_target": "openai-codex"}`)
 	if rec.Code != http.StatusOK {
 		t.Fatalf("set: expected 200, got %d: %s", rec.Code, rec.Body.String())
@@ -3079,6 +3084,16 @@ func TestPostApiPoolsNameAuthResetTarget(t *testing.T) {
 	if p.AuthResetTarget == nil || *p.AuthResetTarget != "openai-codex" {
 		t.Fatalf("response AuthResetTarget = %v, want openai-codex", p.AuthResetTarget)
 	}
+	// Schema completeness: the OpenAPI Pool schema requires name+strategy+members.
+	if p.Name != "pool1" {
+		t.Errorf("200 body name = %q, want pool1", p.Name)
+	}
+	if p.Strategy != store.PoolStrategyFailover {
+		t.Errorf("200 body strategy = %q, want %q", p.Strategy, store.PoolStrategyFailover)
+	}
+	if len(p.Members) != 2 {
+		t.Fatalf("200 body members = %+v, want 2 (credA, credB)", p.Members)
+	}
 	// Reached the store (no inline logic; routed through poolops).
 	got, err := st.GetPool("pool1")
 	if err != nil {
@@ -3147,6 +3162,16 @@ func TestPostApiPools_WithAuthResetTarget(t *testing.T) {
 	if rec.Code != http.StatusCreated {
 		t.Fatalf("expected 201, got %d: %s", rec.Code, rec.Body.String())
 	}
+	// The 201 response body must reflect the configured target. The synthetic
+	// store.Pool the handler builds when the read-back is skipped/fails used to
+	// omit it (Copilot #1), so assert it round-trips through the JSON body.
+	var p api.Pool
+	if err := json.NewDecoder(rec.Body).Decode(&p); err != nil {
+		t.Fatalf("decode 201 body: %v", err)
+	}
+	if p.AuthResetTarget == nil || *p.AuthResetTarget != "openai-codex" {
+		t.Fatalf("201 body AuthResetTarget = %v, want openai-codex", p.AuthResetTarget)
+	}
 	got, err := st.GetPool("codex")
 	if err != nil {
 		t.Fatalf("GetPool: %v", err)
@@ -3178,7 +3203,7 @@ func TestDeleteApiPoolsName_Success(t *testing.T) {
 	st := newTestStore(t)
 	enableHTTPChannel(t, st)
 	seedOAuthCred(t, st, "credA", "credB")
-	if err := st.CreatePoolWithMembers("pool1", store.PoolStrategyFailover, []string{"credA", "credB"}); err != nil {
+	if err := st.CreatePoolWithMembers("pool1", store.PoolStrategyFailover, []string{"credA", "credB"}, ""); err != nil {
 		t.Fatalf("create pool: %v", err)
 	}
 	srv := api.NewServer(st, nil, nil, "")
@@ -3225,7 +3250,7 @@ func TestDeleteApiPoolsName_ReferencedByBinding(t *testing.T) {
 	st := newTestStore(t)
 	enableHTTPChannel(t, st)
 	seedOAuthCred(t, st, "credA", "credB")
-	if err := st.CreatePoolWithMembers("pool1", store.PoolStrategyFailover, []string{"credA", "credB"}); err != nil {
+	if err := st.CreatePoolWithMembers("pool1", store.PoolStrategyFailover, []string{"credA", "credB"}, ""); err != nil {
 		t.Fatalf("create pool: %v", err)
 	}
 	// A binding referencing the pool by name keeps it from being removed.

internal/poolops/poolops.go

@@ -24,7 +24,7 @@ import (
 // interface lets each channel pass its own already-open store and lets the
 // tests substitute a fake.
 type Store interface {
-	CreatePoolWithMembers(name, strategy string, members []string) error
+	CreatePoolWithMembers(name, strategy string, members []string, authResetTarget string) error
 	GetPool(name string) (*store.Pool, error)
 	ListPools() ([]store.Pool, error)
 	RemovePoolIfUnreferenced(name string) (bool, error)
@@ -144,10 +144,13 @@ func Create(s Store, name, strategy string, members []string) error {
 }
 
 // CreateWithAuthResetTarget is Create plus an optional per-pool
-// auth_reset_target (empty = no reset). The target is set in a follow-up
-// SetPoolAuthResetTarget call after the pool exists; channels that don't
-// accept a target call Create. Used by every channel's create adapter so the
-// create-with-target path has a single source of truth.
+// auth_reset_target (empty = no reset). The target is bound in the same store
+// transaction that creates the pool and its members, so create-with-target is
+// atomic: a partial state where the pool exists without its target can never
+// be observed, and there is no second write whose failure would leave a
+// created pool plus an error (which a retry would then 409 on). Channels that
+// don't accept a target call Create, which threads "". Used by every channel's
+// create adapter so the create-with-target path has a single source of truth.
 func CreateWithAuthResetTarget(s Store, name, strategy string, members []string, authResetTarget string) error {
 	if strategy == "" {
 		strategy = store.PoolStrategyFailover
@@ -158,15 +161,7 @@ func CreateWithAuthResetTarget(s Store, name, strategy string, members []string,
 	if err := validateAuthResetTarget(authResetTarget); err != nil {
 		return err
 	}
-	if err := s.CreatePoolWithMembers(name, strategy, members); err != nil {
-		return err
-	}
-	if authResetTarget != "" {
-		if err := s.SetPoolAuthResetTarget(name, authResetTarget); err != nil {
-			return err
-		}
-	}
-	return nil
+	return s.CreatePoolWithMembers(name, strategy, members, authResetTarget)
 }
 
 // SetAuthResetTarget sets (target != "") or clears (target == "") the

internal/poolops/poolops_test.go

@@ -109,6 +109,11 @@ func TestCreateListStatusRotateRemove(t *testing.T) {
 	}
 }
 
+// TestCreateWithAuthResetTarget asserts create-with-target persists the target
+// in the single CreatePoolWithMembers call (the target is bound in the same
+// store transaction that creates the pool, not a separate follow-up
+// SetPoolAuthResetTarget write — Copilot #5). Both the GetPool read-back and
+// the derived Status reflect the configured target.
 func TestCreateWithAuthResetTarget(t *testing.T) {
 	db := newTestStore(t, "acct_a", "acct_b")
 

internal/store/pools.go

@@ -196,7 +196,10 @@ func assertCredentialNotInAnotherPoolTx(tx *sql.Tx, credential, newPool string)
 // existing oauth credential with a token_url. At least two members are
 // required for failover to be meaningful, but a single-member pool is
 // permitted (it degrades to a plain indirection with no failover target).
-func (s *Store) CreatePoolWithMembers(name, strategy string, members []string) error {
+// authResetTarget is stored verbatim in the same transaction (empty = none),
+// so create-with-target is atomic: there is no window where the pool exists
+// without its configured target.
+func (s *Store) CreatePoolWithMembers(name, strategy string, members []string, authResetTarget string) error {
 	if name == "" {
 		return fmt.Errorf("%w: pool name is required", ErrPoolNoMembers)
 	}
@@ -253,7 +256,7 @@ func (s *Store) CreatePoolWithMembers(name, strategy string, members []string) e
 	}
 
 	if _, err := tx.Exec(
-		"INSERT INTO credential_pools (name, strategy, auth_reset_target) VALUES (?, ?, '')", name, strategy,
+		"INSERT INTO credential_pools (name, strategy, auth_reset_target) VALUES (?, ?, ?)", name, strategy, authResetTarget,
 	); err != nil {
 		return fmt.Errorf("insert pool %q: %w", name, err)
 	}