Address review and lint on drift detection walker

Fix lint failures from CI: rename reflect.Ptr to reflect.Pointer,
suppress exhaustive on the kind switch with rationale, and tag the
,inline json fixtures as a known revive false positive.

Replace the explicit leafTypes allowlist with a json.Marshaler interface
check. Every entry in the previous map shared one property — a custom
MarshalJSON whose output bears no relation to the Go field layout — so
asking the type how it serializes is more general than maintaining a
hand-rolled list of K8s and project types. The walker is now
self-maintaining for any future custom-marshaled type.

Address reviewer findings on telemetry_drift_test.go:
  - delete dead sortedKeys helper and its misleading comment
  - upgrade per-entry empty-field checks to require to avoid empty-string
    pollution of the duplicate-detection maps
  - assert no path appears in both ignore maps (cross-pollination)
  - assert every mapping/ignore entry is still a live leaf on its type
    so renames and deletions surface instead of leaving stale entries
  - rewrite caCertPath, sensitiveHeaders, and serviceName justifications
    to name the actual wiring symbols a reviewer can grep for

Tighten the FlattenJSONLeafFields godoc to describe the one-level
self-reference expansion that maxStructRevisits actually permits.

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

Author: ChrisJBurns

cmd/thv-operator/internal/testutil/reflect.go

@@ -10,12 +10,8 @@ import (
 	"reflect"
 	"sort"
 	"strings"
-	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
-	thvjson "github.com/stacklok/toolhive/pkg/json"
-	vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config"
 )
 
 // FlattenJSONLeafFields returns every leaf JSON field path under t as a sorted
@@ -35,20 +31,29 @@ import (
 //     `tools` slice has a `workload` field".
 //   - Map types whose VALUE type is a struct (or pointer to a struct) recurse
 //     into that value type the same way.
-//   - All other types (primitives, []string, map[string]string,
-//     time.Duration, metav1.Duration, json.RawMessage, and any type listed
-//     in the leaf allowlist below) terminate the walk and contribute one
-//     leaf path entry.
-//   - To prevent infinite recursion on self-referential types, track visited
-//     types per walk and stop if a cycle is detected.
+//   - Types that implement json.Marshaler are treated as leaves regardless of
+//     their underlying kind. A custom MarshalJSON produces a JSON shape that
+//     bears no relation to the Go field layout, so walking the fields would
+//     emit paths that never appear in the serialized form. This handles
+//     metav1.Time, metav1.Duration, intstr.IntOrString, resource.Quantity,
+//     json.RawMessage, and any project-local custom-marshaled types
+//     (e.g. pkg/json.Map, pkg/json.Any, pkg/vmcp/config.Duration) without an
+//     explicit allow-list.
+//   - All other primitive types (primitives, []string, map[string]string,
+//     time.Duration which is just int64) terminate the walk and contribute
+//     one leaf path entry via the default branch.
+//   - Self-referential types are expanded one level (the original visit plus
+//     one self-reference) and then truncated, to keep the walk terminating.
+//     Subsequent levels (e.g. "next.next.<field>") are intentionally elided.
+//     See maxStructRevisits for the controlling constant.
 //
 // If t is nil or, after dereferencing, is not a struct, an empty slice is
 // returned rather than panicking.
 func FlattenJSONLeafFields(t reflect.Type) []string {
 	if t == nil {
 		return []string{}
 	}
-	for t.Kind() == reflect.Ptr {
+	for t.Kind() == reflect.Pointer {
 		t = t.Elem()
 	}
 	if t.Kind() != reflect.Struct {
@@ -67,15 +72,16 @@ func FlattenJSONLeafFields(t reflect.Type) []string {
 	return out
 }
 
-// leafTypes lists types that terminate the walk even though they are structs.
-// They contribute a single leaf entry at their parent path.
-var leafTypes = map[reflect.Type]struct{}{
-	reflect.TypeOf(time.Duration(0)):       {},
-	reflect.TypeOf(metav1.Duration{}):      {},
-	reflect.TypeOf(json.RawMessage{}):      {},
-	reflect.TypeOf(thvjson.Map{}):          {},
-	reflect.TypeOf(thvjson.Any{}):          {},
-	reflect.TypeOf(vmcpconfig.Duration(0)): {},
+// jsonMarshalerType is the reflect.Type of the json.Marshaler interface. Any
+// type implementing it (directly or via pointer receiver) produces a JSON
+// shape detached from its Go field layout, so it must terminate the walk.
+var jsonMarshalerType = reflect.TypeOf((*json.Marshaler)(nil)).Elem()
+
+// implementsJSONMarshaler reports whether values of t (or *t) have a custom
+// MarshalJSON method. The pointer-receiver check matters because Go method
+// sets only include pointer-receiver methods when the receiver is addressable.
+func implementsJSONMarshaler(t reflect.Type) bool {
+	return t.Implements(jsonMarshalerType) || reflect.PointerTo(t).Implements(jsonMarshalerType)
 }
 
 // skipFieldTypes lists embedded struct types that must be skipped entirely.
@@ -136,36 +142,40 @@ func walkStruct(t reflect.Type, prefix string, leafSet map[string]struct{}, visi
 // recurses into nested structs/slices/maps or records a leaf at prefix.
 func walkType(t reflect.Type, prefix string, leafSet map[string]struct{}, visited map[reflect.Type]int) {
 	// Deref pointers.
-	for t.Kind() == reflect.Ptr {
+	for t.Kind() == reflect.Pointer {
 		t = t.Elem()
 	}
 
-	// Allow-listed leaf types short-circuit any further walking.
-	if _, ok := leafTypes[t]; ok {
+	// Custom JSON marshalers short-circuit any further walking. See
+	// jsonMarshalerType for the rationale.
+	if implementsJSONMarshaler(t) {
 		recordLeaf(prefix, leafSet)
 		return
 	}
 
-	switch t.Kind() {
+	// Only Struct/Slice/Array/Map require recursion; every other Kind
+	// (primitives, interfaces, channels, etc.) is a leaf path captured by
+	// the default branch.
+	switch t.Kind() { //exhaustive:ignore
 	case reflect.Struct:
 		recurseStruct(t, prefix, leafSet, visited)
 	case reflect.Slice, reflect.Array:
 		elem := t.Elem()
-		for elem.Kind() == reflect.Ptr {
+		for elem.Kind() == reflect.Pointer {
 			elem = elem.Elem()
 		}
-		// Recurse only when the element is a struct that is NOT a leaf type.
-		if _, isLeaf := leafTypes[elem]; !isLeaf && elem.Kind() == reflect.Struct {
+		// Recurse only when the element is a plain struct (no custom marshaler).
+		if elem.Kind() == reflect.Struct && !implementsJSONMarshaler(elem) {
 			recurseStruct(elem, prefix, leafSet, visited)
 			return
 		}
 		recordLeaf(prefix, leafSet)
 	case reflect.Map:
 		val := t.Elem()
-		for val.Kind() == reflect.Ptr {
+		for val.Kind() == reflect.Pointer {
 			val = val.Elem()
 		}
-		if _, isLeaf := leafTypes[val]; !isLeaf && val.Kind() == reflect.Struct {
+		if val.Kind() == reflect.Struct && !implementsJSONMarshaler(val) {
 			recurseStruct(val, prefix, leafSet, visited)
 			return
 		}

cmd/thv-operator/internal/testutil/reflect_test.go

@@ -81,8 +81,8 @@ type withEmbeddedNoInline struct {
 }
 
 type withEmbeddedInline struct {
-	embedSource `json:",inline"`
-	Bar         string `json:"bar"`
+	embedSource `json:",inline"` //nolint:revive // inline is a valid kubernetes json tag option
+	Bar         string           `json:"bar"`
 }
 
 type withUnexportedField struct {
@@ -266,7 +266,7 @@ func TestFlattenJSONLeafFields_SkipsObjectMetaAndTypeMeta(t *testing.T) {
 	t.Parallel()
 
 	type withK8sMeta struct {
-		metav1.TypeMeta   `json:",inline"`
+		metav1.TypeMeta   `json:",inline"` //nolint:revive // inline is a valid kubernetes json tag option
 		metav1.ObjectMeta `json:"metadata,omitempty"`
 		Spec              flatPrimitives `json:"spec"`
 	}

cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go

@@ -25,10 +25,10 @@ package spectoconfig
 
 import (
 	"reflect"
-	"sort"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
 	"github.com/stacklok/toolhive/cmd/thv-operator/internal/testutil"
@@ -61,22 +61,27 @@ var telemetryFieldMappings = []FieldMapping{
 // telemetryIgnoredOnCRDOnly lists CRD leaf fields that intentionally have no
 // runtime counterpart. Each entry MUST include a justification.
 var telemetryIgnoredOnCRDOnly = map[string]string{
-	"openTelemetry.enabled":                            "CRD-only gate; controls whether the converter populates runtime fields at all",
-	"openTelemetry.sensitiveHeaders.name":              "K8s-secret-backed headers; resolved by operator into runtime Headers",
-	"openTelemetry.sensitiveHeaders.secretKeyRef.name": "K8s-secret-backed headers; resolved by operator into runtime Headers",
-	"openTelemetry.sensitiveHeaders.secretKeyRef.key":  "K8s-secret-backed headers; resolved by operator into runtime Headers",
-	"openTelemetry.caBundleRef.configMapRef.name":      "K8s ConfigMap reference; resolved by operator into runtime CACertPath",
-	"openTelemetry.caBundleRef.configMapRef.key":       "K8s ConfigMap reference; resolved by operator into runtime CACertPath",
-	"openTelemetry.caBundleRef.configMapRef.optional":  "K8s ConfigMap reference flag promoted from corev1.ConfigMapKeySelector; not part of runtime config",
+	"openTelemetry.enabled": "CRD-only gate; controls whether the converter populates runtime fields at all",
+	"openTelemetry.sensitiveHeaders.name": "K8s-secret-backed header value; injected as TOOLHIVE_OTEL_HEADER_* env vars on the proxyrunner pod " +
+		"(controllerutil.GenerateOpenTelemetryEnvVarsFromRef) and merged into OTLP headers at runtime; not written into telemetry.Config.Headers by the converter",
+	"openTelemetry.sensitiveHeaders.secretKeyRef.name": "K8s-secret-backed header value; injected as TOOLHIVE_OTEL_HEADER_* env vars on the proxyrunner pod " +
+		"(controllerutil.GenerateOpenTelemetryEnvVarsFromRef) and merged into OTLP headers at runtime; not written into telemetry.Config.Headers by the converter",
+	"openTelemetry.sensitiveHeaders.secretKeyRef.key": "K8s-secret-backed header value; injected as TOOLHIVE_OTEL_HEADER_* env vars on the proxyrunner pod " +
+		"(controllerutil.GenerateOpenTelemetryEnvVarsFromRef) and merged into OTLP headers at runtime; not written into telemetry.Config.Headers by the converter",
+	"openTelemetry.caBundleRef.configMapRef.name":     "K8s ConfigMap reference; resolved by operator into runtime CACertPath",
+	"openTelemetry.caBundleRef.configMapRef.key":      "K8s ConfigMap reference; resolved by operator into runtime CACertPath",
+	"openTelemetry.caBundleRef.configMapRef.optional": "K8s ConfigMap reference flag promoted from corev1.ConfigMapKeySelector; not part of runtime config",
 }
 
 // telemetryIgnoredOnRuntimeOnly lists runtime leaf fields that intentionally
 // have no CRD counterpart. Each entry MUST include a justification.
 var telemetryIgnoredOnRuntimeOnly = map[string]string{
-	"serviceName":          "per-server, set via MCPTelemetryConfigReference.ServiceName, not stored on the shared MCPTelemetryConfig",
+	"serviceName": "per-server, set from MCPTelemetryConfigReference.ServiceName and defaulted at runtime by " +
+		"telemetry.ResolveServiceName; intentionally absent from the shared MCPTelemetryConfig",
 	"serviceVersion":       "resolved at runtime from binary version (issue #2296)",
 	"environmentVariables": "CLI-only, not applicable to CRD-managed telemetry",
-	"caCertPath":           "filesystem path computed by the operator from caBundleRef; not user-facing in the CRD",
+	"caCertPath": "filesystem path injected by runconfig.AppendTelemetryRunnerOption after the operator computes the volume mount " +
+		"path from openTelemetry.caBundleRef; not user-facing in the CRD",
 }
 
 // TestTelemetryConfigDrift_CRDFieldsCovered walks MCPTelemetryConfigSpec and
@@ -144,9 +149,12 @@ func TestTelemetryConfigDrift_MappingTableSanity(t *testing.T) {
 	seenCRD := make(map[string]int, len(telemetryFieldMappings))
 	seenRuntime := make(map[string]int, len(telemetryFieldMappings))
 
+	// Use require for the per-entry NotEmpty checks so that an empty CRD
+	// or Runtime field doesn't pollute the duplicate maps below with an
+	// empty-string key — that would trigger a misleading cascade.
 	for i, m := range telemetryFieldMappings {
-		assert.NotEmptyf(t, m.CRD, "telemetryFieldMappings[%d].CRD must not be empty", i)
-		assert.NotEmptyf(t, m.Runtime, "telemetryFieldMappings[%d].Runtime must not be empty", i)
+		require.NotEmptyf(t, m.CRD, "telemetryFieldMappings[%d].CRD must not be empty", i)
+		require.NotEmptyf(t, m.Runtime, "telemetryFieldMappings[%d].Runtime must not be empty", i)
 		seenCRD[m.CRD]++
 		seenRuntime[m.Runtime]++
 	}
@@ -176,17 +184,49 @@ func TestTelemetryConfigDrift_MappingTableSanity(t *testing.T) {
 		assert.NotEmptyf(t, reason, "telemetryIgnoredOnRuntimeOnly[%q] must include a justification", path)
 	}
 
-	// Stable iteration not strictly necessary for correctness, but it keeps
-	// failure output deterministic when assertions fire on multiple keys.
-	_ = sortedKeys(seenCRD)
-	_ = sortedKeys(seenRuntime)
+	// A leaf path can't be classified two different ways. An entry in both
+	// ignore maps is a copy-paste mistake when shifting a field across the
+	// boundary — fail loudly instead of silently allowing the contradiction.
+	for path := range telemetryIgnoredOnCRDOnly {
+		if _, dup := telemetryIgnoredOnRuntimeOnly[path]; dup {
+			t.Errorf("path %q is listed in BOTH telemetryIgnoredOnCRDOnly and telemetryIgnoredOnRuntimeOnly", path)
+		}
+	}
+
+	// Every path in the mapping/ignore tables must still be a live leaf on
+	// its respective type. Catches stale entries left behind by field
+	// renames or deletions, which would otherwise mask the rename.
+	crdLeaves := liveLeafSet(reflect.TypeOf(v1beta1.MCPTelemetryConfigSpec{}))
+	for _, m := range telemetryFieldMappings {
+		if _, live := crdLeaves[m.CRD]; !live {
+			t.Errorf("telemetryFieldMappings entry %q is not a live leaf on v1beta1.MCPTelemetryConfigSpec — stale entry?", m.CRD)
+		}
+	}
+	for path := range telemetryIgnoredOnCRDOnly {
+		if _, live := crdLeaves[path]; !live {
+			t.Errorf("telemetryIgnoredOnCRDOnly entry %q is not a live leaf on v1beta1.MCPTelemetryConfigSpec — stale entry?", path)
+		}
+	}
+	runtimeLeaves := liveLeafSet(reflect.TypeOf(telemetry.Config{}))
+	for _, m := range telemetryFieldMappings {
+		if _, live := runtimeLeaves[m.Runtime]; !live {
+			t.Errorf("telemetryFieldMappings entry %q is not a live leaf on telemetry.Config — stale entry?", m.Runtime)
+		}
+	}
+	for path := range telemetryIgnoredOnRuntimeOnly {
+		if _, live := runtimeLeaves[path]; !live {
+			t.Errorf("telemetryIgnoredOnRuntimeOnly entry %q is not a live leaf on telemetry.Config — stale entry?", path)
+		}
+	}
 }
 
-func sortedKeys[V any](m map[string]V) []string {
-	out := make([]string, 0, len(m))
-	for k := range m {
-		out = append(out, k)
+// liveLeafSet returns the set of leaf paths reachable from t, for use in
+// stale-entry checks against the drift mapping/ignore tables.
+func liveLeafSet(t reflect.Type) map[string]struct{} {
+	leaves := testutil.FlattenJSONLeafFields(t)
+	out := make(map[string]struct{}, len(leaves))
+	for _, l := range leaves {
+		out[l] = struct{}{}
 	}
-	sort.Strings(out)
 	return out
 }