chore(rotation): address Copilot review on PR #128

Nine findings from the Copilot review on PR #128, all valid or
worth addressing. Fixed in this commit; replies + thread-resolves
go with the push.

1. LoadOrCreateKeyRing fall-through (keyring.go:131-167)
   Was: any error reading the legacy api-key file (incl. ErrKeyRequired
   from a missing encryption-key env, or a permission error) silently
   fell through to generating a fresh keyring — would rotate every
   dashboard out for a transient operator mistake.
   Now: distinguish "file doesn't exist" (proceed to fresh-gen) from
   "file exists but errored / malformed" (return the error to the
   caller). os.Stat + os.IsNotExist gates the choice explicitly.

2. MatchToken constant-time guarantee (keyring.go ~195)
   Was: the prefixed-token path returned early on the first kid match,
   making total runtime depend on which kid the request claimed — kid
   enumeration via timing. The doc said "All comparisons are
   constant-time" but the prefixed branch broke that promise.
   Now: walk every entry, compare both kid and secret with
   subtle.ConstantTimeCompare, AND the two results. Match is recorded
   without short-circuit; runtime is uniform regardless of which kid
   (if any) matches. Doc updated to reflect the actual guarantee.

3. writeKeyRingFile mutates input (keyring.go ~415)
   Was: the function populated SecretHex on each entry of the passed-
   in keyring before marshaling. KeyRing values are shared via
   atomic.Pointer and treated as immutable; mutating in-place risks
   races with concurrent middleware readers.
   Now: marshal off a local snapshot whose entries have SecretHex
   backfilled from Secret where needed. Input is never written to.

4. --rotate-api-key zero CreatedAt (main.go ~155)
   Was: the fresh KeyRingEntry built for the CLI rotate command
   omitted CreatedAt, so the keyring file got 0001-01-01T00:00:00Z
   and the /api/v1/system/keyring metadata exposed the same.
   Now: CreatedAt: time.Now().UTC().

5. handleGet nil-guard (api/keyring.go ~50)
   Was: h.keyring.Load() was dereferenced unconditionally; a future
   wiring bug that left the pointer nil would panic the agent on
   every keyring request.
   Now: nil check + 500 + log line. Fails loud rather than crashing.

6. At-most-one-primary-per-box constraint (migration 000002)
   Was: nothing in the schema stopped two rows with role='primary'
   for the same box. SetBoxPrimaryKey's transactional flip is
   correct, but a buggy code path or a manual DB edit could produce
   the invalid state and GetBoxPrimaryKey would return an arbitrary
   row.
   Now: partial unique index on box_agent_keys(box_id) WHERE
   role='primary'. SQLite supports this directly; index is dropped
   in the down migration too.

7. Test naming clarity (box_agent_keys_test.go)
   Was: TestBoxAgentKeys_MigrationBackfillsLegacyEntry was named as
   if it validated migration behaviour but actually only exercised
   InsertBoxAgentKey + GetBoxPrimaryKey roundtrip; the comment also
   misled.
   Now: split into two clearly-named tests —
   InsertAndLookup covers the roundtrip, and a new
   MigrationBackfillStatementWorks test wipes the migrated rows for
   a single box, re-executes the migration's INSERT-FROM-boxes
   statement, and asserts the row appears + reruns are idempotent.

8. DeleteBox cascade gap (servers.go DeleteBox)
   Was: the schema declared ON DELETE CASCADE but PRAGMA
   foreign_keys is off in this codebase, so deleting a box left
   orphaned box_agent_keys rows holding encrypted secrets. Phase 1
   docs flagged this as a deferred gap; Copilot pushed back, and
   fairly — it's a small, contained fix.
   Now: DeleteBox runs inside a transaction that wipes
   box_agent_keys WHERE box_id = ? before deleting from boxes. Both
   succeed or neither does.
   Test re-added: TestBoxAgentKeys_DeleteBoxClearsDependentKeys.

9. APIKeyAuth nil-guard (middleware/auth.go)
   Was: keyring.Load() was called without first checking the
   pointer itself for nil. A miswired ServerConfig would panic on
   every authenticated request.
   Now: fail-closed nil check at the top of the request handler —
   returns 401 + logs at error level. Same defensive treatment as
   fix #5.

Tests
-----

All 3 dashboard-side suites pass (`database` package, 7 new tests
including the new DeleteBox cascade test). All 3 agent-side suites
pass (`crypto`, `middleware`, `api`).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: sarg3nt

gearbox-agent/cmd/gearbox-agent/main.go

@@ -159,9 +159,10 @@ func main() {
 			os.Exit(1)
 		}
 		fresh := crypto.KeyRingEntry{
-			KID:    kid,
-			Secret: secret,
-			Role:   "primary",
+			KID:       kid,
+			Secret:    secret,
+			Role:      "primary",
+			CreatedAt: time.Now().UTC(),
 		}
 		kr.Entries = []crypto.KeyRingEntry{fresh}
 		if err := crypto.SaveKeyRing(cfg.KeyRingPath, kr); err != nil {

gearbox-agent/internal/api/keyring.go

@@ -48,6 +48,15 @@ type keyRingResponse struct {
 //	@Router			/api/v1/system/keyring [get]
 func (h *KeyRingHandler) handleGet(w http.ResponseWriter, _ *http.Request) {
 	kr := h.keyring.Load()
+	if kr == nil {
+		// Should be unreachable — main.go calls NewKeyRingPointer(kr)
+		// with a non-nil value before mounting the handler. Defensive
+		// 500 + log so a future wiring bug fails loud rather than
+		// panicking the agent on every keyring request.
+		h.logger.Error("keyring pointer empty when serving /system/keyring")
+		http.Error(w, "keyring unavailable", http.StatusInternalServerError)
+		return
+	}
 	resp := keyRingResponse{
 		Version: kr.Version,
 		Entries: kr.Snapshot(),

gearbox-agent/internal/framework/crypto/keyring.go

@@ -109,25 +109,49 @@ func LoadOrCreateKeyRing(path, legacyAPIKeyPath string) (*KeyRing, bool, error)
 	}
 
 	// No keyring file. Migrate from legacy api_key file if present.
+	//
+	// Failure modes we distinguish:
+	//   - legacy file doesn't exist        → fall through to fresh-gen
+	//   - legacy file exists but errors    → fatal (caller may have set
+	//                                        GEARBOX_AGENT_ENCRYPTION_KEY
+	//                                        wrong; silently generating
+	//                                        a fresh keyring would lock
+	//                                        the dashboard out for no
+	//                                        good reason)
+	//   - legacy file exists but is malformed (not 64 hex chars) →
+	//                                        fatal for the same reason
+	//                                        (a wedged file is operator-
+	//                                        visible; a silent rotation
+	//                                        is not).
 	if legacyAPIKeyPath != "" {
-		if legacyKey, err := ReadAPIKey(legacyAPIKeyPath); err == nil {
+		if _, statErr := os.Stat(legacyAPIKeyPath); statErr == nil {
+			legacyKey, rerr := ReadAPIKey(legacyAPIKeyPath)
+			if rerr != nil {
+				return nil, false, fmt.Errorf("read legacy api-key file %q: %w", legacyAPIKeyPath, rerr)
+			}
 			secret, derr := hex.DecodeString(strings.TrimSpace(legacyKey))
-			if derr == nil && len(secret) == SecretLength {
-				kr := &KeyRing{
-					Version: 1,
-					Entries: []KeyRingEntry{{
-						KID:       "legacy",
-						Secret:    secret,
-						SecretHex: hex.EncodeToString(secret),
-						Role:      "primary",
-						CreatedAt: time.Now().UTC(),
-					}},
-				}
-				if werr := writeKeyRingFile(path, kr); werr != nil {
-					return nil, false, fmt.Errorf("write migrated keyring: %w", werr)
-				}
-				return kr, false, nil
+			if derr != nil {
+				return nil, false, fmt.Errorf("legacy api-key file %q is not valid hex: %w", legacyAPIKeyPath, derr)
+			}
+			if len(secret) != SecretLength {
+				return nil, false, fmt.Errorf("legacy api-key file %q has secret length %d, want %d", legacyAPIKeyPath, len(secret), SecretLength)
+			}
+			kr := &KeyRing{
+				Version: 1,
+				Entries: []KeyRingEntry{{
+					KID:       "legacy",
+					Secret:    secret,
+					SecretHex: hex.EncodeToString(secret),
+					Role:      "primary",
+					CreatedAt: time.Now().UTC(),
+				}},
 			}
+			if werr := writeKeyRingFile(path, kr); werr != nil {
+				return nil, false, fmt.Errorf("write migrated keyring: %w", werr)
+			}
+			return kr, false, nil
+		} else if !os.IsNotExist(statErr) {
+			return nil, false, fmt.Errorf("stat legacy api-key file %q: %w", legacyAPIKeyPath, statErr)
 		}
 	}
 
@@ -180,7 +204,14 @@ func (kr *KeyRing) Primary() *KeyRingEntry {
 // 64-hex) and returns the matching entry, or nil + ErrUnknownKID / nil +
 // ErrInvalidToken on failure.
 //
-// All comparisons are constant-time.
+// Comparisons against entry secrets are constant-time (subtle.
+// ConstantTimeCompare). The prefixed-token path walks every entry and
+// performs the compare on each one regardless of whether the kid
+// matched, so total runtime doesn't reveal which kids exist on this
+// agent — kids are not strictly secret (they're sent in the request
+// header), but leaking which ones an agent currently accepts via
+// timing makes rotation-history enumeration cheap, which we'd rather
+// not.
 func (kr *KeyRing) MatchToken(token string) (*KeyRingEntry, error) {
 	if strings.HasPrefix(token, tokenPrefix) {
 		// Prefixed: gbx_<kid>_<b64secret>
@@ -195,18 +226,22 @@ func (kr *KeyRing) MatchToken(token string) (*KeyRingEntry, error) {
 		if err != nil || len(secret) != SecretLength {
 			return nil, ErrInvalidToken
 		}
+		// Walk every entry, compare every secret, record the match
+		// without short-circuiting. The two ConstantTimeEq calls plus
+		// the bitwise AND keep the timing uniform regardless of which
+		// entry (if any) matches.
+		var matched *KeyRingEntry
 		for i := range kr.Entries {
-			if kr.Entries[i].KID != kid {
-				continue
-			}
-			if subtle.ConstantTimeCompare(kr.Entries[i].Secret, secret) == 1 {
-				return &kr.Entries[i], nil
+			kidEq := subtle.ConstantTimeCompare([]byte(kr.Entries[i].KID), []byte(kid))
+			secretEq := subtle.ConstantTimeCompare(kr.Entries[i].Secret, secret)
+			if kidEq&secretEq == 1 {
+				matched = &kr.Entries[i]
 			}
-			// Same kid but secret doesn't match — bail without trying other
-			// entries; the caller's kid claim is wrong.
+		}
+		if matched == nil {
 			return nil, ErrUnknownKID
 		}
-		return nil, ErrUnknownKID
+		return matched, nil
 	}
 
 	// Legacy: 64 hex chars, brute-compare against every entry's secret.
@@ -410,15 +445,22 @@ func hydrateSecrets(kr *KeyRing) error {
 
 // writeKeyRingFile serializes kr to JSON and atomically writes to path,
 // encrypting with the default KeyProvider when configured.
+//
+// Does NOT mutate the input keyring. KeyRing values are shared via
+// [*atomic.Pointer] and treated as immutable; populating SecretHex
+// directly on the live entries would race with concurrent readers in
+// the auth middleware. We marshal off a local snapshot whose entries
+// have SecretHex backfilled from Secret where needed.
 func writeKeyRingFile(path string, kr *KeyRing) error {
-	// Ensure every entry has SecretHex populated for serialization.
-	for i := range kr.Entries {
-		if kr.Entries[i].SecretHex == "" {
-			kr.Entries[i].SecretHex = hex.EncodeToString(kr.Entries[i].Secret)
+	snapshot := &KeyRing{Version: kr.Version, Entries: make([]KeyRingEntry, len(kr.Entries))}
+	for i, e := range kr.Entries {
+		snapshot.Entries[i] = e
+		if snapshot.Entries[i].SecretHex == "" {
+			snapshot.Entries[i].SecretHex = hex.EncodeToString(e.Secret)
 		}
 	}
 
-	plaintext, err := json.MarshalIndent(kr, "", "  ")
+	plaintext, err := json.MarshalIndent(snapshot, "", "  ")
 	if err != nil {
 		return fmt.Errorf("marshal keyring: %w", err)
 	}

gearbox-agent/internal/framework/middleware/auth.go

@@ -34,6 +34,17 @@ const ResponseHeaderKID = "X-Gearbox-Kid"
 func APIKeyAuth(keyring *crypto.KeyRingPointer, logger *slog.Logger, backoff *BackoffTracker) func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			// Fail-closed nil guard. Should be unreachable — the
+			// agent's main.go always passes a non-nil keyring pointer
+			// before this middleware is mounted — but a future wiring
+			// bug should 401 every request instead of panicking the
+			// agent on the auth path.
+			if keyring == nil {
+				logger.Error("AUTH DENIED: middleware constructed with nil keyring pointer", "remote_addr", r.RemoteAddr)
+				http.Error(w, "Unauthorized", http.StatusUnauthorized)
+				return
+			}
+
 			ip := clientIPFromRemoteAddr(r.RemoteAddr)
 
 			if backoff != nil && backoff.IsBlocked(ip) {

gearbox/internal/framework/database/box_agent_keys_test.go

@@ -19,17 +19,17 @@ func newTestBox(t *testing.T, db *DB, boxIDStr, name string) *BoxDB {
 	return box
 }
 
-func TestBoxAgentKeys_MigrationBackfillsLegacyEntry(t *testing.T) {
+// TestBoxAgentKeys_InsertAndLookup exercises the Insert/Get round-
+// trip — what the rotator (Phase 2) and the per-box rotate handler
+// (Phase 3) rely on for a freshly-created box.
+//
+// This does NOT exercise the migration's backfill INSERT-FROM-boxes
+// path; see TestBoxAgentKeys_MigrationBackfillStatementWorks below
+// for that.
+func TestBoxAgentKeys_InsertAndLookup(t *testing.T) {
 	db := setupTestDB(t)
-	// Insert a box BEFORE we look at the keyring; the schema-init path
-	// runs the box_agent_keys backfill on every startup, but for a row
-	// inserted afterwards the backfill won't fire. Phase 2's create-box
-	// handler is responsible for seating the legacy entry on NEW boxes;
-	// for now, verify the rotation-table is reachable end-to-end via the
-	// direct insert path.
 	box := newTestBox(t, db, "box-a", "Box A")
 
-	// Direct insert simulates what Phase 2's create-box path will do.
 	if err := db.InsertBoxAgentKey(&BoxAgentKey{
 		BoxID:           box.ID,
 		KID:             "legacy",
@@ -48,6 +48,53 @@ func TestBoxAgentKeys_MigrationBackfillsLegacyEntry(t *testing.T) {
 	}
 }
 
+// TestBoxAgentKeys_MigrationBackfillStatementWorks exercises the
+// idempotent INSERT-FROM-boxes statement from migration 000002. We
+// can't easily replay the migration on a per-test DB (it only runs
+// once), so this test wipes the migrated rows for a single box, then
+// re-executes the same backfill SQL against the live DB and asserts
+// the expected rows appear. Catches regressions to the SQL itself.
+func TestBoxAgentKeys_MigrationBackfillStatementWorks(t *testing.T) {
+	db := setupTestDB(t)
+	box := newTestBox(t, db, "box-mig", "Migration Box")
+
+	rawDB := db.GetDB()
+	if _, err := rawDB.Exec(`DELETE FROM box_agent_keys WHERE box_id = ?`, box.ID); err != nil {
+		t.Fatalf("wipe: %v", err)
+	}
+
+	const backfillSQL = `
+		INSERT INTO box_agent_keys (box_id, kid, secret_encrypted, role, created_at)
+		SELECT id, 'legacy', api_key_encrypted, 'primary', created_at
+		FROM boxes
+		WHERE id = ? AND NOT EXISTS (
+		    SELECT 1 FROM box_agent_keys WHERE box_id = boxes.id AND kid = 'legacy'
+		)`
+	if _, err := rawDB.Exec(backfillSQL, box.ID); err != nil {
+		t.Fatalf("backfill: %v", err)
+	}
+
+	primary, err := db.GetBoxPrimaryKey(box.ID)
+	if err != nil {
+		t.Fatalf("GetBoxPrimaryKey: %v", err)
+	}
+	if primary == nil || primary.KID != "legacy" {
+		t.Fatalf("backfill: got %+v, want kid=legacy primary", primary)
+	}
+
+	// Re-running the backfill must be a no-op (idempotency guard).
+	if _, err := rawDB.Exec(backfillSQL, box.ID); err != nil {
+		t.Fatalf("backfill rerun: %v", err)
+	}
+	keys, err := db.GetBoxAgentKeys(box.ID)
+	if err != nil {
+		t.Fatalf("GetBoxAgentKeys: %v", err)
+	}
+	if len(keys) != 1 {
+		t.Errorf("backfill rerun was not idempotent: %d rows, want 1", len(keys))
+	}
+}
+
 func TestBoxAgentKeys_SetPrimary_FlipsRolesAtomically(t *testing.T) {
 	db := setupTestDB(t)
 	box := newTestBox(t, db, "box-b", "Box B")
@@ -143,11 +190,34 @@ func TestBoxAgentKeys_TouchLastUsed(t *testing.T) {
 	}
 }
 
-// Cascade-on-DeleteBox is currently declared in the schema (FOREIGN KEY
-// (box_id) REFERENCES boxes(id) ON DELETE CASCADE) but not enforced —
-// the gearbox DB doesn't set `PRAGMA foreign_keys = ON`. Enabling that
-// pragma is a broader change that risks regressing on legacy rows
-// elsewhere in the schema. For now DeleteBox leaves orphan
-// box_agent_keys rows on disk; Phase 2's box-delete path will clean
-// them up explicitly. Not tested here so the gap is visible from the
-// test surface itself.
+// TestBoxAgentKeys_DeleteBoxClearsDependentKeys verifies that
+// DeleteBox removes rows from box_agent_keys for that box, even
+// though SQLite's PRAGMA foreign_keys is off in this codebase (so
+// the schema-declared CASCADE doesn't run). DeleteBox now wipes
+// dependents inside its transaction, which is what this test pins.
+func TestBoxAgentKeys_DeleteBoxClearsDependentKeys(t *testing.T) {
+	db := setupTestDB(t)
+	box := newTestBox(t, db, "box-f", "Box F")
+	if err := db.InsertBoxAgentKey(&BoxAgentKey{
+		BoxID: box.ID, KID: "legacy", SecretEncrypted: []byte("e"), Role: "primary",
+	}); err != nil {
+		t.Fatalf("insert: %v", err)
+	}
+	if err := db.InsertBoxAgentKey(&BoxAgentKey{
+		BoxID: box.ID, KID: "v2", SecretEncrypted: []byte("e2"), Role: "secondary",
+	}); err != nil {
+		t.Fatalf("insert v2: %v", err)
+	}
+
+	if err := db.DeleteBox(box.ID); err != nil {
+		t.Fatalf("DeleteBox: %v", err)
+	}
+
+	keys, err := db.GetBoxAgentKeys(box.ID)
+	if err != nil {
+		t.Fatalf("GetBoxAgentKeys: %v", err)
+	}
+	if len(keys) != 0 {
+		t.Errorf("keys not removed on box delete: %+v", keys)
+	}
+}

gearbox/internal/framework/database/migrations/files/000002_add_box_agent_keys.down.sql

@@ -1,3 +1,4 @@
+DROP INDEX IF EXISTS idx_box_agent_keys_one_primary;
 DROP INDEX IF EXISTS idx_box_agent_keys_role;
 DROP INDEX IF EXISTS idx_box_agent_keys_box;
 DROP TABLE IF EXISTS box_agent_keys;

gearbox/internal/framework/database/migrations/files/000002_add_box_agent_keys.up.sql

@@ -34,6 +34,14 @@ CREATE INDEX IF NOT EXISTS idx_box_agent_keys_box
 CREATE INDEX IF NOT EXISTS idx_box_agent_keys_role
     ON box_agent_keys(box_id, role);
 
+-- Enforce the "at most one primary per box" invariant the rotator
+-- relies on. Without this constraint a buggy SetBoxPrimaryKey path
+-- could leave two primaries on the same box and GetBoxPrimaryKey
+-- would return an arbitrary one. Partial-unique-index is the
+-- SQLite-supported way to express "unique only when role='primary'".
+CREATE UNIQUE INDEX IF NOT EXISTS idx_box_agent_keys_one_primary
+    ON box_agent_keys(box_id) WHERE role = 'primary';
+
 -- Backfill: every existing box gets a legacy entry that's a copy of its
 -- current `api_key_encrypted` value, marked primary. Idempotent — the
 -- WHERE NOT EXISTS guard means re-running the migration on a partially-

gearbox/internal/framework/database/servers.go

@@ -297,13 +297,29 @@ func (d *DB) UpdateBox(box *BoxDB) error {
 	return nil
 }
 
-// DeleteBox deletes a box configuration from the database.
+// DeleteBox deletes a box configuration from the database, along with
+// any rotation-keyring entries that reference it.
+//
+// The schema declares ON DELETE CASCADE on box_agent_keys.box_id, but
+// this codebase doesn't set PRAGMA foreign_keys=ON (enabling it is a
+// broader change that risks regressing on legacy rows elsewhere). To
+// avoid leaving orphaned rows that hold encrypted secrets, the
+// dependent table is wiped explicitly inside the same transaction.
 func (d *DB) DeleteBox(id int64) error {
 	d.mu.Lock()
 	defer d.mu.Unlock()
 
-	query := `DELETE FROM boxes WHERE id = ?`
-	result, err := d.db.Exec(query, id)
+	tx, err := d.db.Begin()
+	if err != nil {
+		return fmt.Errorf("begin tx: %w", err)
+	}
+	defer func() { _ = tx.Rollback() }()
+
+	if _, err := tx.Exec(`DELETE FROM box_agent_keys WHERE box_id = ?`, id); err != nil {
+		return fmt.Errorf("delete dependent keys: %w", err)
+	}
+
+	result, err := tx.Exec(`DELETE FROM boxes WHERE id = ?`, id)
 	if err != nil {
 		return fmt.Errorf("failed to delete box: %w", err)
 	}
@@ -316,7 +332,7 @@ func (d *DB) DeleteBox(id int64) error {
 		return fmt.Errorf("box not found")
 	}
 
-	return nil
+	return tx.Commit()
 }
 
 // SetBoxEnabled enables or disables a box.